feat: In-Chat Messages for Voice Calls #37378
Conversation
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: f2eb01b The changes in this PR will be included in the next version bump. This PR includes changesets to release 42 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 |
WalkthroughAdds call history data structures, persistence, and in-chat history messages: new CallHistory types and model, model registration, MediaCallServer API/event and CallDirector trigger, MediaCallService logic to persist history and post formatted Info Card messages to DMs, i18n entries, and unit tests for payload formatting. Changes
Sequence Diagram(s)sequenceDiagram
participant CallDirector as Call Director
participant MediaCallServer as Media Call Server
participant MediaCallService as Media Call Service
participant CallHistoryDB as CallHistory DB
participant Rooms as Rooms Service
participant MessageSvc as Message Service
CallDirector->>MediaCallServer: hangupCallById(callId)
MediaCallServer->>MediaCallServer: updateCallHistory({ callId })
MediaCallServer->>MediaCallService: emit historyUpdate(callId)
rect `#E8F4FF`
note over MediaCallService: validate call ended & participants\ncompute duration & state
MediaCallService->>MediaCallService: getCallDuration / getCallHistoryItemState
MediaCallService->>Rooms: getRoomIdForInternalCall(call)
end
rect `#E9FBEF`
note over MediaCallService,CallHistoryDB: persist outbound & inbound history items
MediaCallService->>CallHistoryDB: insert outbound record
MediaCallService->>CallHistoryDB: insert inbound record
end
rect `#FFF8E6`
note over MediaCallService,MessageSvc: build Info Card and send DM, then link messageId
MediaCallService->>MessageSvc: getHistoryMessagePayload + sendMessage
MessageSvc-->>MediaCallService: messageId
MediaCallService->>CallHistoryDB: update records with messageId
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (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)
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #37378 +/- ##
===========================================
- Coverage 68.97% 68.91% -0.07%
===========================================
Files 3359 3360 +1
Lines 114214 114282 +68
Branches 20535 20562 +27
===========================================
- Hits 78784 78760 -24
- Misses 33335 33430 +95
+ Partials 2095 2092 -3
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/15 22:28", "11/16 01:28", "11/17 23:50", "11/18 22:53", "11/19 21:05", "11/19 22:19 (PR)"]
y-axis "Size (GB)" 0 --> 0.5
line "account-service" [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]
line "ddp-streamer-service" [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.14]
line "presence-service" [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.14]
line "rocketchat" [0.36, 0.36, 0.35, 0.35, 0.35, 0.36]
line "stream-hub-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
Statistics (last 5 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Wed, 19 Nov 2025 22:19:52 GMT |
Co-authored-by: gabriellsh <henriques.gabriell@gmail.com> Co-authored-by: gabriellsh <40830821+gabriellsh@users.noreply.github.com>
![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 (6)
apps/meteor/server/services/media-call/getHistoryMessagePayload.spec.ts (2)
1-3: Avoid duplicatingAPP_IDliteral in testsHardcoding
const appId = 'media-call-core';couples the spec to the exact string value. IfAPP_IDchanges ingetHistoryMessagePayload.ts, all these tests will break even though the rest of the shape is still valid.Consider either:
- Exporting
APP_IDfromgetHistoryMessagePayload.tsand importing it here, or- Asserting
result.blocks[0].appIdagainst the literal in a single focused test, and usingexpect.objectContainingfor other payload shape checks.
105-278: Comprehensive payload shape tests; consider slightly loosening deep equalsThe payload tests thoroughly check:
- Per-state icon/translation rows, and
- Presence/absence and styling of the duration row across states.
Because you're using full
toEqualon the entire object, any small structural change (e.g., adding a new optional field toInfoCardBlock) will force wide test updates. You might want to useexpect.objectContainingfor inner objects, while still asserting the critical fields (icon, i18n key, duration text, row backgrounds).apps/meteor/server/services/media-call/getHistoryMessagePayload.ts (2)
7-20: *Clean up TODO and rely on _bold i18n keys for emphasis
callStateToTranslationKeyis already using theCall_*_boldkeys, which are expected to carry the emphasis (single-asterisk formatting) in the translation itself. The// TODO bold the textcomment is now misleading and could encourage adding extra markup on top of the i18n formatting.I’d suggest removing or updating that TODO to avoid double‑bolding via manual asterisks.
Also, since
CallHistoryItemStateis a union, you might consider adding adefaultcase that throws or returns a safe fallback to catch any future state additions at runtime.Based on learnings
58-91: History payload shape looks correct; TODO about “proper translation keys” is stale
getHistoryMessagePayloadcleanly composes:
- An
info_cardblock scoped underAPP_ID,- A first row with the state icon and translated text, and
- An optional secondary row for the formatted duration.
Two minor points:
- The
// TODO proper translation keyscomment seems outdated now thatcallStateToTranslationKeyalready returns the finalized i18n keys; consider removing or clarifying it.- This payload currently carries no explicit identifiers (e.g.,
callId) in the block itself; instead, you rely on associating the message_idback toCallHistory. If the client UX for “click to open Call History entry” depends on the block shape alone, confirm that the front end is indeed usingappId: 'media-call-core'+ messageId to resolve the history entry.apps/meteor/server/services/media-call/service.ts (2)
70-89: Internal-only history guard is sensible; consider clarifying external/group behavior
saveCallToHistory:
- Validates the call exists and is
ended.- Skips calls where
uids.length !== 2, effectively limiting history + summary messaging to 1:1 internal calls for now.This matches the current “internal DM call” scope, but it also means:
- Group calls or external calls will not produce history entries or summary messages yet.
Given the
// TODO: save external media calls to history, it may be worth expanding that comment to explicitly mention group calls or additional scenarios, so the behavior is clear to future readers.
91-133: History record insertion looks correct; Promise usage could be simplified
saveInternalCallToHistory:
- Correctly asserts both sides are
usercontacts.- Resolves/creates a DM room (if possible) and uses its
ridfor both history entries.- Computes
stateanddurationonce and fans them out into symmetric outbound/inboundCallHistoryrecords for caller and callee.Two minor observations:
- You’re wrapping each
CallHistory.insertOnein its own.catchand then passing those promises intoPromise.allSettled. Because errors are already caught and logged,allSettleddoesn’t add much over a plainPromise.allhere. You could either:
- Drop
.catchand actually inspect theallSettledresults, or- Keep the
.catchand replacePromise.allSettledwithPromise.all(or evenvoid Promise.all([...])) for simplicity.- If
getRoomIdForInternalCallfails, you still persist history (withoutrid) but skip the summary message. That’s a reasonable degradation; you might want to mention this in a comment if that fallback is intentional.
📜 Review details
Configuration used: CodeRabbit 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 (16)
apps/meteor/jest.config.ts(1 hunks)apps/meteor/server/models.ts(2 hunks)apps/meteor/server/services/media-call/getHistoryMessagePayload.spec.ts(1 hunks)apps/meteor/server/services/media-call/getHistoryMessagePayload.ts(1 hunks)apps/meteor/server/services/media-call/service.ts(3 hunks)ee/packages/media-calls/src/definition/IMediaCallServer.ts(2 hunks)ee/packages/media-calls/src/server/CallDirector.ts(2 hunks)ee/packages/media-calls/src/server/MediaCallServer.ts(1 hunks)packages/core-typings/src/ICallHistoryItem.ts(1 hunks)packages/core-typings/src/index.ts(1 hunks)packages/i18n/src/locales/en.i18n.json(1 hunks)packages/model-typings/src/index.ts(1 hunks)packages/model-typings/src/models/ICallHistoryModel.ts(1 hunks)packages/models/src/index.ts(2 hunks)packages/models/src/modelClasses.ts(1 hunks)packages/models/src/models/CallHistory.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: packages/i18n/src/locales/en.i18n.json:918-921
Timestamp: 2025-11-19T18:20:07.683Z
Learning: Repo: RocketChat/Rocket.Chat — i18n/formatting
Learning: This repository uses a custom message formatting parser in UI blocks/messages; do not assume standard Markdown rules. For keys like Call_ended_bold, Call_not_answered_bold, Call_failed_bold, and Call_transferred_bold in packages/i18n/src/locales/en.i18n.json, retain the existing single-asterisk emphasis unless maintainers request otherwise.
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.096Z
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.
📚 Learning: 2025-11-19T18:20:37.096Z
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.096Z
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:
ee/packages/media-calls/src/server/MediaCallServer.tsapps/meteor/server/models.tsee/packages/media-calls/src/definition/IMediaCallServer.tsapps/meteor/server/services/media-call/getHistoryMessagePayload.spec.tsapps/meteor/server/services/media-call/getHistoryMessagePayload.tspackages/core-typings/src/ICallHistoryItem.tsee/packages/media-calls/src/server/CallDirector.tsapps/meteor/server/services/media-call/service.ts
📚 Learning: 2025-11-19T18:20:07.683Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: packages/i18n/src/locales/en.i18n.json:918-921
Timestamp: 2025-11-19T18:20:07.683Z
Learning: Repo: RocketChat/Rocket.Chat — i18n/formatting
Learning: This repository uses a custom message formatting parser in UI blocks/messages; do not assume standard Markdown rules. For keys like Call_ended_bold, Call_not_answered_bold, Call_failed_bold, and Call_transferred_bold in packages/i18n/src/locales/en.i18n.json, retain the existing single-asterisk emphasis unless maintainers request otherwise.
Applied to files:
apps/meteor/server/services/media-call/getHistoryMessagePayload.tspackages/i18n/src/locales/en.i18n.json
📚 Learning: 2025-11-17T22:38:48.631Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37505
File: packages/i18n/src/locales/en.i18n.json:3765-3765
Timestamp: 2025-11-17T22:38:48.631Z
Learning: Rocket.Chat i18n copy: Keep sentence case for the value of "Notification_Desktop_show_voice_calls" in packages/i18n/src/locales/en.i18n.json (“Show desktop notifications for voice calls”) per design directive; do not change to Title Case even if nearby labels differ.
Applied to files:
packages/i18n/src/locales/en.i18n.json
📚 Learning: 2025-11-19T12:32:29.673Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37547
File: packages/i18n/src/locales/en.i18n.json:634-634
Timestamp: 2025-11-19T12:32:29.673Z
Learning: Repo: RocketChat/Rocket.Chat
Context: i18n workflow
Learning: In this repository, new translation keys should be added to packages/i18n/src/locales/en.i18n.json only; other locale files are populated via the external translation pipeline and/or fall back to English. Do not request adding the same key to all locale files in future reviews.
Applied to files:
packages/i18n/src/locales/en.i18n.json
🧬 Code graph analysis (10)
ee/packages/media-calls/src/server/MediaCallServer.ts (1)
ee/packages/media-calls/src/logger.ts (1)
logger(3-3)
apps/meteor/server/models.ts (2)
packages/models/src/index.ts (1)
registerModel(138-138)packages/models/src/models/CallHistory.ts (1)
CallHistoryRaw(7-18)
packages/model-typings/src/models/ICallHistoryModel.ts (1)
packages/core-typings/src/ICallHistoryItem.ts (1)
CallHistoryItem(48-48)
apps/meteor/server/services/media-call/getHistoryMessagePayload.spec.ts (1)
apps/meteor/server/services/media-call/getHistoryMessagePayload.ts (4)
callStateToTranslationKey(8-20)callStateToIcon(22-34)getFormattedCallDuration(40-56)getHistoryMessagePayload(59-91)
apps/meteor/server/services/media-call/getHistoryMessagePayload.ts (3)
packages/core-typings/src/ICallHistoryItem.ts (1)
CallHistoryItemState(6-16)packages/core-typings/src/IMessage/IMessage.ts (1)
IMessage(162-260)packages/fuselage-ui-kit/src/surfaces/FuselageSurfaceRenderer.tsx (1)
icon(361-367)
packages/core-typings/src/ICallHistoryItem.ts (3)
packages/core-typings/src/IUser.ts (1)
IUser(186-255)packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)packages/core-typings/src/IMessage/IMessage.ts (1)
IMessage(162-260)
packages/models/src/index.ts (2)
packages/core-services/src/index.ts (1)
proxify(134-134)packages/model-typings/src/models/ICallHistoryModel.ts (1)
ICallHistoryModel(5-5)
ee/packages/media-calls/src/server/CallDirector.ts (1)
ee/packages/media-calls/src/server/injection.ts (1)
getMediaCallServer(25-32)
apps/meteor/server/services/media-call/service.ts (4)
packages/core-typings/src/mediaCalls/IMediaCall.ts (1)
IMediaCall(35-68)packages/models/src/index.ts (4)
MediaCalls(186-186)CallHistory(149-149)Rooms(204-204)Users(213-213)packages/core-typings/src/ICallHistoryItem.ts (2)
IInternalMediaCallHistoryItem(37-43)CallHistoryItemState(6-16)apps/meteor/server/services/media-call/getHistoryMessagePayload.ts (1)
getHistoryMessagePayload(59-91)
packages/models/src/models/CallHistory.ts (2)
packages/core-typings/src/ICallHistoryItem.ts (1)
CallHistoryItem(48-48)packages/model-typings/src/models/ICallHistoryModel.ts (1)
ICallHistoryModel(5-5)
⏰ 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 (25)
apps/meteor/jest.config.ts (1)
46-46: LGTM!The testMatch pattern correctly includes the new media-call test suite.
apps/meteor/server/models.ts (2)
12-12: LGTM!The import follows the existing pattern in the file.
99-99: LGTM!The model registration is correct and follows the established pattern for other models in this file.
ee/packages/media-calls/src/server/MediaCallServer.ts (1)
68-72: LGTM!The
updateCallHistorymethod follows the same pattern asreportCallUpdateand correctly emits thehistoryUpdateevent.packages/model-typings/src/index.ts (1)
89-89: LGTM!The export follows the established pattern for model type definitions.
packages/models/src/modelClasses.ts (1)
81-81: LGTM!The export follows the established pattern for model classes.
ee/packages/media-calls/src/server/CallDirector.ts (2)
6-6: LGTM!The import is necessary for the new history update functionality.
371-377: Verification complete: all callers ofhangupCallByIdproperly handle the return value.All four call sites check the boolean return value:
- Line 28:
if (modified)- Line 361: extracts
result.modifiedCountand checksif (ended)- Line 385:
if (!modified || !agents?.length)- Line 402:
if (!modified)The implementation correctly triggers
updateCallHistoryonly after successful hangup, consistent with AC1.packages/model-typings/src/models/ICallHistoryModel.ts (1)
1-5: LGTM!The type alias follows the standard pattern for model type definitions and correctly bridges the core typings to the model layer.
packages/core-typings/src/ICallHistoryItem.ts (1)
1-48: Verification confirms safe handling of optional fields.The downstream logic properly manages optional
messageIdandrid:
ridis conditionally included using spread operator (...(rid && { rid })) during history creationmessageIdis intentionally omitted at creation and safely assigned later viaCallHistory.updateManyafter message creation succeeds (line 157)- No unsafe direct property access patterns found in the codebase
- Author field logic at line 141 correctly uses
call.caller.id || call.createdBy?.idfor transferred callspackages/core-typings/src/index.ts (1)
149-151: ExposeICallHistoryItemvia core typings barrelThe new export is consistent with the existing barrel pattern and correctly surfaces call-history typings to consumers.
packages/models/src/index.ts (2)
88-96: ICallHistoryModel import looks consistent with existing model-typings usageAdding
ICallHistoryModelto the existing model-typings import block matches the established pattern for other models. Just ensure the type alias in@rocket.chat/model-typingsis exported under exactly this name.If you want, you can double-check with a quick grep that
ICallHistoryModelis exported frompackages/model-typings/src/index.tsand defined inmodels/ICallHistoryModel.ts.
141-151: NewCallHistoryproxified model is wired like other models
CallHistory = proxify<ICallHistoryModel>('ICallHistoryModel')follows the same pattern as the other model proxies and gives services a typed façade over the raw model.Please confirm that:
apps/meteor/server/models.tsregisters'ICallHistoryModel'withCallHistoryRaw, and- the string key
'ICallHistoryModel'matches the one used inpackages/model-typings/src/models/ICallHistoryModel.ts.ee/packages/media-calls/src/definition/IMediaCallServer.ts (1)
7-11:historyUpdateevent andupdateCallHistorytrigger are coherent with existing IMediaCallServer shapeThe new
historyUpdate: { callId: string }event and theupdateCallHistory(params: { callId: string }): voidmethod fit cleanly into the existing event/trigger pattern (similar tocallUpdated/reportCallUpdate), and keeping the payload tocallIdlets listeners look up full state as needed.Please verify that:
MediaCallServer’s implementation ofupdateCallHistoryactually emits thehistoryUpdateevent onemitter, and- the media-call service (or other consumer) subscribes to
historyUpdateto perform the call-history persistence and message posting, so the new interface is fully exercised.Also applies to: 37-44
packages/models/src/models/CallHistory.ts (1)
1-18: CallHistoryRaw model and indexes align with expected access patternsThe new
CallHistoryRawmodel cleanly follows the existingBaseRawpattern, and the chosen indexes make sense for per-user call history:
{ uid: 1, callId: 1 }unique: enforces at most one history entry per user per call.{ uid: 1, ts: -1 }: supports efficient “recent calls for user” queries sorted by time.Implementation looks correct and matches how other models are structured.
packages/i18n/src/locales/en.i18n.json (1)
918-921: Keys verified and correctly mapped; code readyVerification confirms all four bold i18n keys are properly defined and mapped through the implementation and test suite. Single-asterisk emphasis is correct per the repository's custom message formatter.
apps/meteor/server/services/media-call/getHistoryMessagePayload.spec.ts (2)
4-56: State-to-translation/icon mapping tests look solidThese specs exercise all defined
CallHistoryItemStatevalues and verify both the translation keys and icon variants, including the shared mapping forfailed/error. This gives good protection against regressions in the mapping helpers.
58-103: Nice edge coverage for duration formattingThe
getFormattedCallDurationtests cover undefined/zero, short calls, exact minute, multi-hour, and large durations, including zero-padding. This should make future refactors to the underlying time logic much safer.apps/meteor/server/services/media-call/getHistoryMessagePayload.ts (2)
22-34: Icon mapping is clear and consistentThe icon and variant choices for each call state (phone-off, clock, phone-issue, arrow-forward) are self-explanatory and align well with the state semantics, including sharing the same treatment for
failedanderror.
36-56: Duration formatting behavior is reasonable; zero/undefined handled well
getFormattedCallDuration:
- Ignores
undefinedand zero (no duration row),- Uses
intervalToDurationfor robust hour/minute/second extraction, and- Produces either
MM:SSorHH:MM:SSwith zero-padding, wrapped in single asterisks.Given that
getCallDurationreturns0for non‑activated calls, this nicely avoids showing a duration for ringing/not‑answered/failed cases while still formatting active calls correctly.apps/meteor/server/services/media-call/service.ts (5)
23-24:historyUpdatewiring and ended-call guard align with “post-only-after-end” requirementSubscribing to
callServer.emitter.on('historyUpdate', …)and then guarding oncall.endedinsaveCallToHistoryensures that summary/history work is only attempted for concluded calls, satisfying the “no summary during ringing/active call” constraint, as long ashistoryUpdateis emitted at or after call completion.
135-165: History message author selection matches product guidance
sendHistoryMessage:
- Finds the DM room and message author as
const userId = call.caller.id || call.createdBy?.id;, which ensures the history message comes from one of the two call participants, not from a transfer agent.- Builds
stateandduration, usesgetHistoryMessagePayload, and sends viasendMessage.- Backfills
messageIdinto allCallHistoryentries for the call when a message is successfully created, with robust error logging if sending fails.This aligns with the retrieved guidance about not using
transferredByas the author. The only edge case to watch is any scenario wherecall.caller.idmight be missing; in such a case the fallback tocreatedBy?.idwould emit the message as a non-participant, but that should not happen for the internal-user-only calls you gate earlier.Based on learnings
167-175: Duration computation matches formatter expectations
getCallDuration:
- Returns
0whenactivatedAtis missing, which naturally suppresses the duration row ingetFormattedCallDuration.- Otherwise computes
(endedAt - activatedAt)in seconds with floor, which is appropriate for user-facing call duration.This pairs cleanly with the formatter logic and state mapping (non-activated calls are “not-answered” or “failed” and show no duration).
177-199: State derivation logic covers all defined outcomes
getCallHistoryItemStateimplements:
transferredwhentransferredByis set (even if there’s also an error reason),failedvserrorsplit based on presence ofactivatedAtfor error-like hangups,not-answeredwhenacceptedAtis never set, andfailedvsendedfor accepted calls that never activate vs fully active calls.This mapping is consistent with the
CallHistoryItemStatedocumentation and supports the required outcomes: Ended, Not Answered, and technical failures.
201-224: DM room resolution is robust; small behavior notes
getRoomIdForInternalCall:
- Reuses an existing direct room when available.
- Otherwise:
- Picks a DM creator (
createdBy.id→caller.id→uids[0]),- Resolves usernames for the two participants, enforcing exactly two,
- Uses
createDirectMessage(usernames, dmCreatorId, !dmCreatorIsPartOfTheCall)to optionally exclude a non-participant creator from the DM membership.Behavior notes:
- If usernames cannot be resolved, this throws and is caught upstream, leading to history entries without
ridand no summary message, which seems like a reasonable fallback.- Because you derive usernames from
Users.findByIds(call.uids), you rely on those user records staying consistent withuids. If that invariant ever breaks, you’ll hit the “Invalid usernames for DM.” error; logging here is important (and already present upstream).Overall, the room resolution logic looks sound for 1:1 internal calls.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/meteor/server/services/media-call/service.ts (1)
140-140: Clarify or remove the inline comment.The comment is informal and might confuse future maintainers. Either expand it to explain why
caller.idis preferred for transferred calls or remove it entirely since the learning already documents this decision.Apply this diff to clarify:
- const userId = call.caller.id || call.createdBy?.id; // I think this should always be the caller, since during a transfer the createdBy contact is the one that transferred the call + // Use caller.id as message author; for transferred calls, message should appear in DM between original participants + const userId = call.caller.id || call.createdBy?.id;Or simply remove the comment if the logic is self-evident.
Based on learnings
📜 Review details
Configuration used: CodeRabbit 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 (2)
apps/meteor/server/services/media-call/getHistoryMessagePayload.ts(1 hunks)apps/meteor/server/services/media-call/service.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: packages/i18n/src/locales/en.i18n.json:918-921
Timestamp: 2025-11-19T18:20:07.683Z
Learning: Repo: RocketChat/Rocket.Chat — i18n/formatting
Learning: This repository uses a custom message formatting parser in UI blocks/messages; do not assume standard Markdown rules. For keys like Call_ended_bold, Call_not_answered_bold, Call_failed_bold, and Call_transferred_bold in packages/i18n/src/locales/en.i18n.json, retain the existing single-asterisk emphasis unless maintainers request otherwise.
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.096Z
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.
📚 Learning: 2025-11-19T18:20:37.096Z
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.096Z
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/server/services/media-call/getHistoryMessagePayload.tsapps/meteor/server/services/media-call/service.ts
📚 Learning: 2025-11-19T18:20:07.683Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: packages/i18n/src/locales/en.i18n.json:918-921
Timestamp: 2025-11-19T18:20:07.683Z
Learning: Repo: RocketChat/Rocket.Chat — i18n/formatting
Learning: This repository uses a custom message formatting parser in UI blocks/messages; do not assume standard Markdown rules. For keys like Call_ended_bold, Call_not_answered_bold, Call_failed_bold, and Call_transferred_bold in packages/i18n/src/locales/en.i18n.json, retain the existing single-asterisk emphasis unless maintainers request otherwise.
Applied to files:
apps/meteor/server/services/media-call/getHistoryMessagePayload.ts
🧬 Code graph analysis (2)
apps/meteor/server/services/media-call/getHistoryMessagePayload.ts (2)
packages/core-typings/src/ICallHistoryItem.ts (1)
CallHistoryItemState(6-16)packages/core-typings/src/IMessage/IMessage.ts (1)
IMessage(162-260)
apps/meteor/server/services/media-call/service.ts (4)
packages/core-typings/src/mediaCalls/IMediaCall.ts (1)
IMediaCall(35-68)packages/models/src/index.ts (4)
MediaCalls(186-186)CallHistory(149-149)Rooms(204-204)Users(213-213)packages/core-typings/src/ICallHistoryItem.ts (2)
IInternalMediaCallHistoryItem(37-43)CallHistoryItemState(6-16)apps/meteor/server/services/media-call/getHistoryMessagePayload.ts (1)
getHistoryMessagePayload(57-89)
⏰ 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 (10)
apps/meteor/server/services/media-call/getHistoryMessagePayload.ts (4)
7-19: LGTM!The state-to-translation mapping correctly handles all
CallHistoryItemStatevalues. Grouping 'failed' and 'error' under the same i18n key is appropriate for user-facing messages.
21-33: LGTM!Icon and variant selections are semantically appropriate for each call state. The visual distinction between successful outcomes (secondary) and failures (danger) will aid user comprehension.
39-55: LGTM!Duration formatting correctly handles hours conditionally, provides sensible defaults, and guards against invalid inputs. The zero-duration edge case (e.g., instant hangup) is handled gracefully as "00:00".
57-89: LGTM!The payload structure correctly implements a block-based message with InfoCard. The empty
msgstring (line 66) is appropriate—blocks provide the content. Settinggroupable: falseensures call history messages remain distinct rather than being collapsed together.apps/meteor/server/services/media-call/service.ts (6)
1-12: LGTM!Import additions properly support the new history persistence feature. All imported types, models, and utilities are used in the methods below.
23-23: LGTM!Using
setImmediateto defer history persistence is appropriate—it prevents blocking the event emitter while ensuring the save happens after the current call stack completes. This pattern is consistent with line 28.
90-132: LGTM!Robust implementation using
Promise.allSettledto ensure both caller and callee history records are attempted. Error handling at each step and conditional message sending based on room availability are well thought out.
166-174: LGTM!Duration calculation correctly uses
activatedAt(when media started flowing) rather thancreatedAt, and returns 0 for calls that never activated. This aligns with user expectations for call duration.
200-223: LGTM!The DM room resolution and creation logic correctly handles the transfer scenario where the DM creator (transferrer) is not part of the actual call. The fallback chain for
dmCreatorIdand the conditional exclusion logic ensure the DM is created between the actual call participants.
181-181: The hangupReason matching strategy is correct; no changes needed.The
.includes('error')check is safe. Analysis of theCallHangupReasontype definition shows all error-related values end with '-error' (signaling-error, service-error, media-error, input-error), and none of the non-error values (normal, remote, rejected, unavailable, transfer, timeout) contain this substring. The concern about false positives is unfounded given the actual defined values in the codebase.
gabriellsh
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
Proposed changes (including videos or screenshots)
Issue(s)
Feature Branch
VGA-1
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Models & Typings
Tests
Localization
✏️ Tip: You can customize this high-level summary in your review settings.