chore: send a message on the DM when an internal call ends#37419
chore: send a message on the DM when an internal call ends#37419gabriellsh merged 13 commits intofeat/voip-room-messagesfrom
Conversation
|
|
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 |
WalkthroughAdds sending of an info-card history message when internal media calls are saved: new payload builder, duration formatting, icon/translation mappings, service integration to send and link the message ID to call history, tests, typing for messageId, and new i18n keys. Changes
Sequence Diagram(s)sequenceDiagram
actor Service as Call Service
participant History as CallHistory DB
participant Payload as getHistoryMessagePayload
participant Message as sendMessage
Service->>History: Save call history
alt rid exists
Service->>Payload: getHistoryMessagePayload(callState, duration)
Payload-->>Service: InfoCard payload
Service->>Message: sendMessage(to room)
Message-->>Service: messageId
Service->>History: Update call history with messageId
else no rid
Note over Service: No history message sent
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/voip-room-messages #37419 +/- ##
===========================================================
- Coverage 67.99% 67.94% -0.05%
===========================================================
Files 3359 3357 -2
Lines 115377 114981 -396
Branches 20801 20780 -21
===========================================================
- Hits 78448 78123 -325
+ Misses 34246 34170 -76
- Partials 2683 2688 +5
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
…nnel-when-call-end
![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 (4)
apps/meteor/server/services/media-call/getHistoryMessagePayload.spec.ts (1)
1-278: Solid coverage; consider table‑driven tests to reduce duplicationThe spec file exercises all states, duration edge cases, and the full payload structure, which is great. A future improvement would be to switch repeated per‑state tests (for both translation keys, icons, and payloads) to
it.each/ table‑driven patterns so adding a new call state or tweaking icons/text only requires editing a single data table instead of multiple hard‑coded expectations.apps/meteor/server/services/media-call/service.ts (2)
91-133: HardensendHistoryMessagefor observability and idempotencyThe wiring from
saveInternalCallToHistoryintosendHistoryMessageis reasonable, and themessageIdback‑link intoCallHistoryfits the new typing. A couple of improvements would make this more robust:
- When
roomorusercan’t be resolved (Lines 136–146), the function silently returns. Emitting alogger.warnwithcallId,rid, anduserIdin those branches would make it much easier to diagnose “no history DM was sent” issues.- Depending on how often
historyUpdatecan fire for a givencallId,sendHistoryMessagemight send multiple DMs for the same call. You could guard this by checking for an existingmessageIdbefore sending, e.g. only send ifCallHistorydocuments for thatcallIddon’t already havemessageIdset.These are not blockers but would strengthen reliability and debugging.
Also applies to: 135-165
167-175: Clamp negative call durations before converting to seconds
getCallDurationis now used not only for persistence but also for user‑visible duration formatting ingetHistoryMessagePayload. IfendedAtever predatesactivatedAt(clock skew, bad data),diffbecomes negative and will produce unexpected output when fed intointervalToDuration.Consider clamping the difference to zero:
- const diff = endedAt.valueOf() - activatedAt.valueOf(); + const diff = Math.max(0, endedAt.valueOf() - activatedAt.valueOf());This keeps persisted durations and rendered durations sane even under skewed timestamps.
apps/meteor/server/services/media-call/getHistoryMessagePayload.ts (1)
5-21: Payload helpers are coherent; clean up TODOs and consider a text fallbackThe state/icon/duration helpers and
getHistoryMessagePayloadare consistent with the spec tests and produce a clearinfo_cardstructure. Two small improvements to consider:
- The TODOs about “bold the text” and “proper translation keys” (Lines 7 and 58) are now outdated given the
_boldi18n keys and tests. Removing or updating them will avoid confusion for future readers.getHistoryMessagePayloadsetsmsg: ''. If there are any clients that don’t yet support theinfo_cardblock type, these history messages may appear empty. You might want to populatemsgwith a simple translated fallback like “Call ended” / “Call failed” using the same state mapping, so legacy or minimal clients still show something meaningful even without blocks.Functionality is otherwise solid and well-covered by the accompanying spec.
Also applies to: 36-56, 58-91
📜 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 (6)
apps/meteor/jest.config.ts(1 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)packages/core-typings/src/ICallHistoryItem.ts(2 hunks)packages/i18n/src/locales/en.i18n.json(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 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-09-16T13:33:49.237Z
Learnt from: cardoso
Repo: RocketChat/Rocket.Chat PR: 36890
File: apps/meteor/tests/e2e/e2e-encryption/e2ee-otr.spec.ts:21-26
Timestamp: 2025-09-16T13:33:49.237Z
Learning: The im.delete API endpoint accepts either a `roomId` parameter (requiring the actual DM room _id) or a `username` parameter (for the DM partner's username). Constructing slug-like identifiers like `user2${Users.userE2EE.data.username}` doesn't work for this endpoint.
Applied to files:
apps/meteor/server/services/media-call/service.ts
📚 Learning: 2025-09-16T13:33:49.237Z
Learnt from: cardoso
Repo: RocketChat/Rocket.Chat PR: 36890
File: apps/meteor/tests/e2e/e2e-encryption/e2ee-otr.spec.ts:21-26
Timestamp: 2025-09-16T13:33:49.237Z
Learning: In Rocket.Chat test files, the im.delete API endpoint accepts either a `roomId` parameter (requiring the actual DM room _id) or a `username` parameter (for the DM partner's username). It does not accept slug-like constructions such as concatenating usernames together.
Applied to files:
apps/meteor/server/services/media-call/service.ts
🧬 Code graph analysis (4)
packages/core-typings/src/ICallHistoryItem.ts (1)
packages/core-typings/src/IMessage/IMessage.ts (1)
IMessage(162-260)
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 (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 (3)
Rooms(204-204)Users(213-213)CallHistory(149-149)apps/meteor/server/services/media-call/getHistoryMessagePayload.ts (1)
getHistoryMessagePayload(59-91)ee/packages/media-calls/src/logger.ts (1)
logger(3-3)
⏰ 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/jest.config.ts (1)
46-46: LGTM!The addition of the test discovery pattern for media-call service specs is correct and follows the existing pattern style in the server testMatch array. This properly enables Jest to discover and run the new test files introduced in this PR.
packages/core-typings/src/ICallHistoryItem.ts (1)
1-43: messageId linkage type looks correctUsing
messageId?: IMessage['_id']onIInternalMediaCallHistoryItemcleanly models the association created inMediaCallService.sendHistoryMessageand keeps it optional for failure paths. No further changes needed here.apps/meteor/server/services/media-call/service.ts (1)
201-221: createDirectMessage call update looks aligned with new signatureSwitching to
createDirectMessage(usernames, dmCreatorId)and derivingdmCreatorIdfromcreatedBy/caller/uids[0]is consistent with how you already choose the DM creator, and avoids the now‑removed boolean argument. Assuming the method’s exported signature was updated accordingly, this call site looks correct.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 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 (1)
apps/meteor/server/services/media-call/service.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-16T13:33:49.237Z
Learnt from: cardoso
Repo: RocketChat/Rocket.Chat PR: 36890
File: apps/meteor/tests/e2e/e2e-encryption/e2ee-otr.spec.ts:21-26
Timestamp: 2025-09-16T13:33:49.237Z
Learning: The im.delete API endpoint accepts either a `roomId` parameter (requiring the actual DM room _id) or a `username` parameter (for the DM partner's username). Constructing slug-like identifiers like `user2${Users.userE2EE.data.username}` doesn't work for this endpoint.
Applied to files:
apps/meteor/server/services/media-call/service.ts
📚 Learning: 2025-09-16T13:33:49.237Z
Learnt from: cardoso
Repo: RocketChat/Rocket.Chat PR: 36890
File: apps/meteor/tests/e2e/e2e-encryption/e2ee-otr.spec.ts:21-26
Timestamp: 2025-09-16T13:33:49.237Z
Learning: In Rocket.Chat test files, the im.delete API endpoint accepts either a `roomId` parameter (requiring the actual DM room _id) or a `username` parameter (for the DM partner's username). It does not accept slug-like constructions such as concatenating usernames together.
Applied to files:
apps/meteor/server/services/media-call/service.ts
🧬 Code graph analysis (1)
apps/meteor/server/services/media-call/service.ts (3)
packages/core-typings/src/mediaCalls/IMediaCall.ts (1)
IMediaCall(35-68)apps/meteor/server/services/media-call/getHistoryMessagePayload.ts (1)
getHistoryMessagePayload(59-91)ee/packages/media-calls/src/logger.ts (1)
logger(3-3)
⏰ 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/server/services/media-call/service.ts (3)
9-10: LGTM!The new imports are appropriate for the history messaging feature.
130-133: LGTM!The logic correctly sends one history message per call (not per history record) and ensures both CallHistory records are linked to the message.
220-223: No issues found.The logic is correct. The
excludeSelfparameter controls whether the creator (me) is included inroomUsers(line 44 of the implementation shows:const roomUsers = excludeSelf ? users : [me, ...users]). The conditional passing!dmCreatorIsPartOfTheCallproperly excludes the creator only when they are not part of the call, which aligns with the stated intent.
2 participants
Proposed changes (including videos or screenshots)
Issue(s)
VGA-54
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Tests
Chores