regression: federated messages out of order #37999
Conversation
…imestamp and previewUrls
|
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 |
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds explicit ts propagation for federated messages and consolidates previewUrls into an extraInfo object: federation event handlers attach ts, service/type signatures accept ts, and executeSendMessage now accepts an extraInfo object ({ ts?, previewUrls? }) and validates/normalizes message.ts. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Matrix as Matrix Server
participant FedHandler as Federation Event Handler
participant MsgService as MessageService
participant SendMethod as executeSendMessage
participant DB as Database
Matrix->>FedHandler: deliver event (origin_server_ts, content)
note right of FedHandler `#DDEBF7`: build payload\nattach ts = new Date(origin_server_ts)
FedHandler->>MsgService: saveMessageFromFederation(payload, ts)
note right of MsgService `#F3E8FF`: forward ts separately\nconstruct message
MsgService->>SendMethod: executeSendMessage(uid, message, { ts, previewUrls })
alt ts provided by extraInfo
note right of SendMethod `#E6F4EA`: validate/normalize message.ts\napply drift rules (reset if >10s, error if >60s)
end
SendMethod->>DB: persist message (message.ts)
DB-->>SendMethod: ack
SendMethod-->>MsgService: result
MsgService-->>FedHandler: result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
📦 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/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 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 15:59 (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.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, 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, 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.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]
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.11]
line "queue-worker-service" [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, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
line "rocketchat" [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.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.35]
Statistics (last 30 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Mon, 29 Dec 2025 15:59:59 GMT |
ggazzo
force-pushed
the
regression/messages-out-of-order
branch
from
4f3f222 to
be7247d
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-8.0.0 #37999 +/- ##
=================================================
- Coverage 70.67% 70.64% -0.04%
=================================================
Files 3145 3145
Lines 108709 108710 +1
Branches 19514 19534 +20
=================================================
- Hits 76835 76796 -39
- Misses 29880 29908 +28
- Partials 1994 2006 +12
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 5 files
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/app/lib/server/methods/sendMessage.ts">
<violation number="1" location="apps/meteor/app/lib/server/methods/sendMessage.ts:57">
P1: Dead code: The timestamp validation block will never execute. The condition `![now, extraInfo?.ts].filter(Boolean).includes(message.ts)` is always false because `message.ts` is explicitly set to either `now` or `extraInfo.ts` immediately before this check. Any client-provided `message.ts` is overwritten before validation can occur.
If the intent is to bypass validation when `extraInfo.ts` is provided but still validate client-provided timestamps, the logic should preserve the original `message.ts` before overwriting and check that instead.</violation>
</file>
Reply to cubic to teach it or ask questions. Tag @cubic-dev-ai to re-run a review.
![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
🧹 Nitpick comments (1)
apps/meteor/app/lib/server/methods/sendMessage.ts (1)
22-31: Clarify documentation: timestamp IS validated.The comment on Line 27 states "this value overrides the message.ts value without validation," but the code at Lines 57-68 does perform validation (60-second window check and out-of-sync error). Consider updating the documentation to accurately reflect that
extraInfo.tsundergoes validation before use.🔎 Suggested documentation fix
* @param extraInfo - * - ts: The timestamp of the message. the message object already has a ts, but this value is validated and only a window of 10 seconds is allowed to be used. this value overrides the message.ts value without validation. + * - ts: The timestamp of the message. When provided, overrides message.ts. Validated to ensure it's within 60 seconds of server time; timestamps more than 10 seconds off are reset to current server time. + * - previewUrls: Optional array of preview URL strings to attach to the message. *
📜 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 (5)
apps/meteor/app/api/server/v1/chat.tsapps/meteor/app/lib/server/methods/sendMessage.tsapps/meteor/server/services/messages/service.tsee/packages/federation-matrix/src/events/message.tspackages/core-services/src/types/IMessageService.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/api/server/v1/chat.tsapps/meteor/app/lib/server/methods/sendMessage.tsee/packages/federation-matrix/src/events/message.tspackages/core-services/src/types/IMessageService.tsapps/meteor/server/services/messages/service.ts
🧠 Learnings (5)
📓 Common learnings
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
📚 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/api/server/v1/chat.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/api/server/v1/chat.tsapps/meteor/app/lib/server/methods/sendMessage.tsapps/meteor/server/services/messages/service.ts
📚 Learning: 2025-12-09T20:01:00.324Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37532
File: ee/packages/federation-matrix/src/FederationMatrix.ts:920-927
Timestamp: 2025-12-09T20:01:00.324Z
Learning: When reviewing federation invite handling in Rocket.Chat (specifically under ee/packages/federation-matrix), understand that rejecting an invite via federationSDK.rejectInvite() triggers an event-driven cleanup: a leave event is emitted and handled by handleLeave() in ee/packages/federation-matrix/src/events/member.ts, which calls Room.performUserRemoval() to remove the subscription. Do not add explicit cleanup in the reject branch of handleInvite(); rely on the existing leave-event flow for cleanup. If making changes, ensure this invariant remains and that any related paths still funnel cleanup through the leave event to avoid duplicate or missing removals.
Applied to files:
ee/packages/federation-matrix/src/events/message.ts
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
apps/meteor/server/services/messages/service.ts
🧬 Code graph analysis (2)
apps/meteor/app/api/server/v1/chat.ts (1)
apps/meteor/app/lib/server/methods/sendMessage.ts (1)
executeSendMessage(32-130)
apps/meteor/app/lib/server/methods/sendMessage.ts (2)
packages/core-typings/src/IMessage/IMessage.ts (1)
IMessage(145-242)apps/meteor/server/services/messages/service.ts (1)
sendMessage(84-86)
⏰ 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). (1)
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (4)
apps/meteor/app/api/server/v1/chat.ts (1)
437-437: LGTM: Parameter correctly wrapped in extraInfo object.The change aligns with the updated
executeSendMessagesignature that now accepts anextraInfoobject containing optionaltsandpreviewUrlsfields.apps/meteor/server/services/messages/service.ts (1)
88-134: LGTM: Timestamp correctly threaded through to executeSendMessage.The implementation properly accepts the required
tsparameter and passes it via theextraInfoobject toexecuteSendMessage, aligning with the interface definition and the broader timestamp handling changes.packages/core-services/src/types/IMessageService.ts (1)
22-22: Thetsparameter is consistently provided by all callers.All calls to
saveMessageFromFederationin the federation event handlers supply thetsparameter withnew Date(event.origin_server_ts), so the required interface definition is correct. No further action needed.ee/packages/federation-matrix/src/events/message.ts (1)
228-228: No validation needed—federation SDK ensuresorigin_server_tsis always present and in milliseconds.The Matrix spec defines
origin_server_tsas a required integer (milliseconds since epoch) on all federation events. The federation SDK validates events against schemas that enforce this requirement before emitting them to event handlers. Usingnew Date(event.origin_server_ts)is correct and safe.Also applies to: 246-246, 261-261, 364-364, 378-378
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/meteor/app/lib/server/methods/sendMessage.ts (1)
49-69: Date reference comparison is fragile; use value-based comparison.The validation logic on Line 57 relies on JavaScript reference equality (
===) viaArray.includes()to determine whethermessage.tscame fromnoworextraInfo.ts. While this works in the current flow, it's fragile:Risks:
- If
message.tsis cloned or reconstructed anywhere in the call path, the reference check fails even when timestamps are identical, causing unexpected validation behavior- The reliance on object identity is non-obvious and makes the code hard to maintain
- Could cause false positives where identical timestamp values are treated differently based on object references
For the federation use case: This fragility could cause federated messages to unexpectedly fail validation or be modified, undermining the fix for message ordering issues (FB-155).
🔎 Suggested fix using value-based comparison
const now = new Date(); message.ts ??= now; if (extraInfo?.ts) { message.ts = extraInfo.ts; } - if (![now, extraInfo?.ts].filter(Boolean).includes(message.ts)) { + const allowedTimestamps = [now.getTime(), extraInfo?.ts?.getTime()].filter((t): t is number => t !== undefined); + const messageTimestamp = message.ts.getTime(); + + if (!allowedTimestamps.includes(messageTimestamp)) { const tsDiff = Math.abs(moment(message.ts).diff(Date.now())); if (tsDiff > 60000) { throw new Meteor.Error('error-message-ts-out-of-sync', 'Message timestamp is out of sync', { method: 'sendMessage', message_ts: message.ts, server_ts: new Date().getTime(), }); } if (tsDiff > 10000) { message.ts = new Date(); } }This compares numeric timestamp values (milliseconds since epoch) instead of Date object references, making the behavior explicit and robust.
🧹 Nitpick comments (1)
apps/meteor/app/lib/server/methods/sendMessage.ts (1)
22-31: Consider clarifying the documentation.The documentation could more clearly explain the purpose and behavior of
extraInfo.ts:
- Why does
extraInfo.tsbypass validation? (federation trust model)- What's the expected use case for this parameter?
This would help future maintainers understand when to use
extraInfo.tsvs. allowing client-provided timestamps.🔎 Suggested documentation improvement
/** - * * @param uid * @param message * @param extraInfo - * - ts: The timestamp of the message. the message object already has a ts, but this value is validated and only a window of 10 seconds is allowed to be used. this value overrides the message.ts value without validation. - * - * + * - ts: Timestamp for federated messages. Bypasses the normal 10-second validation window + * used for client-provided timestamps. Use when forwarding messages from trusted + * federation sources that need to preserve original timestamps. + * - previewUrls: URLs to generate link previews for * @returns */
📜 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)
apps/meteor/app/lib/server/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/lib/server/methods/sendMessage.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
📚 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/server/methods/sendMessage.ts
🧬 Code graph analysis (1)
apps/meteor/app/lib/server/methods/sendMessage.ts (3)
packages/core-typings/src/IMessage/IMessage.ts (1)
IMessage(145-242)apps/meteor/server/services/messages/service.ts (1)
sendMessage(84-86)apps/meteor/client/views/root/hooks/useIframeCommands.ts (1)
user(81-90)
⏰ 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 (3)
apps/meteor/app/lib/server/methods/sendMessage.ts (3)
32-36: LGTM - signature updated appropriately.The function signature change groups federation-specific timestamp with preview URLs under
extraInfo, which provides a clean extension point for future metadata without adding more parameters.
114-114: LGTM - previewUrls correctly extracted.The change correctly extracts
previewUrlsfrom theextraInfoobject and passes it tosendMessage.
171-171: LGTM - Meteor method correctly adapted.The Meteor method now wraps
previewUrlsin theextraInfoobject format. Note that client-provided timestamps continue to be validated viamessage.ts, whileextraInfo.tsis reserved for internal federation calls—this separation is appropriate for the security model.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/meteor/app/lib/server/methods/sendMessage.ts (2)
22-31: Document thepreviewUrlsproperty in the JSDoc comment.The JSDoc comment documents the
tsproperty ofextraInfobut omitspreviewUrls. For completeness, add a brief description of whatpreviewUrlsdoes.🔎 Suggested documentation addition
/** * * @param uid * @param message * @param extraInfo * - ts: The timestamp of the message. the message object already has a ts, but this value is validated and only a window of 10 seconds is allowed to be used. this value overrides the message.ts value without validation. + * - previewUrls: Optional array of URLs to preview in the message. * * * @returns */
49-64: Consider reusing thenowvariable for consistency.The timestamp validation logic is correct. However, on Line 62,
message.ts = new Date()creates a new Date object instead of reusing thenowvariable defined on Line 50. While the timing difference is negligible, usingnowwould be more consistent.🔎 Suggested consistency improvement
const isTimestampFromClient = Boolean(!extraInfo?.ts && message.ts); const now = new Date(); message.ts = extraInfo?.ts ?? message.ts ?? now; if (isTimestampFromClient) { const tsDiff = Math.abs(moment(message.ts).diff(Date.now())); if (tsDiff > 60000) { throw new Meteor.Error('error-message-ts-out-of-sync', 'Message timestamp is out of sync', { method: 'sendMessage', message_ts: message.ts, server_ts: new Date().getTime(), }); } if (tsDiff > 10000) { - message.ts = new Date(); + message.ts = now; } }
📜 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)
apps/meteor/app/lib/server/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/lib/server/methods/sendMessage.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/server/methods/sendMessage.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
🔇 Additional comments (3)
apps/meteor/app/lib/server/methods/sendMessage.ts (3)
32-36: LGTM!The function signature correctly consolidates
tsandpreviewUrlsinto a singleextraInfoobject, enabling federated messages to provide their own timestamps while maintaining backward compatibility.
109-109: LGTM!The call to
sendMessagecorrectly extracts and passesextraInfo?.previewUrls, maintaining proper integration with the downstream function.
166-166: LGTM!The Meteor method correctly wraps the
previewUrlsparameter into theextraInfoobject structure, properly bridging the external API with the updated internal implementation.
ggazzo
force-pushed
the
regression/messages-out-of-order
branch
from
d10f7e6 to
24d850a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/meteor/app/lib/server/methods/sendMessage.ts (1)
22-31: JSDoc comment slightly misrepresents the validation window.The comment states "only a window of 10 seconds is allowed" but the actual behavior is:
- 0–10s drift: timestamp is kept as-is
- 10–60s drift: timestamp is reset to
now- 60s+ drift: error is thrown
Consider updating to clarify that 60 seconds is the max allowed before rejection, with drift >10 seconds triggering a reset.
📜 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)
apps/meteor/app/lib/server/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/lib/server/methods/sendMessage.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
📚 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/server/methods/sendMessage.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
🔇 Additional comments (4)
apps/meteor/app/lib/server/methods/sendMessage.ts (4)
32-36: LGTM!The consolidation of
tsandpreviewUrlsinto anextraInfoobject is a clean approach that improves extensibility for future parameters without repeated signature changes.
49-64: LGTM!The timestamp handling logic now correctly:
- Trusts
extraInfo.tsfrom federation without validation (critical for preserving message order).- Validates client-provided
message.tswith appropriate drift thresholds.- Falls back to current time when neither is provided.
The
isTimestampFromClientflag cleanly addresses the previous dead-code and reference-comparison issues.
109-109: LGTM!Straightforward adaptation to access
previewUrlsfrom the newextraInfoobject.
165-166: LGTM!Correctly wraps
previewUrlsin the newextraInfostructure while preserving the public Meteor method signature. Client-provided timestamps continue to flow throughmessage.tsand undergo validation as intended.
https://rocketchat.atlassian.net/browse/FB-155
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.