chore: initiate call history with internal calls #37316
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 call-history types, a CallHistory model with indexes and registration, media-call server API/event for history signaling, a call hangup hook to trigger updates, and service logic to compute and persist internal call-history records (duration, state, room, direction). Changes
Sequence Diagram(s)sequenceDiagram
participant CallDirector
participant MediaCallServer
participant MediaCallService
participant CallHistory as CallHistory Model
participant RoomsSvc as Rooms/DM
CallDirector->>MediaCallServer: hangupCallById(callId)
alt call ended
MediaCallServer->>MediaCallServer: updateCallHistory({callId})
MediaCallServer->>MediaCallService: emit historyUpdate(callId)
activate MediaCallService
MediaCallService->>MediaCallService: load call (MediaCalls)
MediaCallService->>MediaCallService: validate internal users (2 UIDs)
MediaCallService->>RoomsSvc: find or create DM for the two users
RoomsSvc-->>MediaCallService: return rid
MediaCallService->>MediaCallService: compute state, duration, endedAt
MediaCallService->>CallHistory: insert outbound record (caller)
MediaCallService->>CallHistory: insert inbound record (callee)
CallHistory-->>MediaCallService: ack
deactivate MediaCallService
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feat/voip-room-messages #37316 +/- ##
===========================================================
+ Coverage 67.92% 67.95% +0.02%
===========================================================
Files 3356 3356
Lines 114886 114943 +57
Branches 20755 20781 +26
===========================================================
+ Hits 78042 78108 +66
+ Misses 34159 34137 -22
- Partials 2685 2698 +13
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
pierre-lehnen-rc
left a comment
pierre-lehnen-rc
left a comment
There was a problem hiding this comment.
reminders for myself.
pierre-lehnen-rc
changed the base branch from
develop
to
feat/voip-room-messages
pierre-lehnen-rc
changed the title
|
Switched base to a feature branch so that this initial PR may be reviewed without the other pieces in place. |
![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 (7)
packages/models/src/models/CallHistory.ts (1)
12-17: Indexes are sensible; consider one more for future queries.Current unique ({ uid, callId }) and sort ({ uid, ts: -1}) look right. If you anticipate call-centric lookups (e.g., troubleshooting or dedup scans), add a lightweight index on { callId: 1 } later. Optional now.
apps/meteor/server/services/media-call/service.ts (5)
21-21: Guard background task with try/catch to avoid silent failures.Wrap saveCallToHistory in try/catch inside setImmediate to ensure any unexpected errors are logged.
- callServer.emitter.on('historyUpdate', ({ callId }) => setImmediate(() => this.saveCallToHistory(callId))); + callServer.emitter.on('historyUpdate', ({ callId }) => + setImmediate(async () => { + try { + await this.saveCallToHistory(callId); + } catch (error) { + logger.error({ msg: 'failed to save call to history', callId, error }); + } + }), + );
81-87: Scope guard looks fine; consider explicit kind/service check.The uids.length !== 2 short-circuit is correct for internal 1:1. Optionally assert call.kind === 'direct' and call.service === 'webrtc' to future‑proof against other call types.
110-124: Insert errors are swallowed; log rejections from allSettled.Duplicate key conflicts are expected occasionally, but other DB errors shouldn’t be silent. Capture rejections for observability.
- await Promise.allSettled([ + const results = await Promise.allSettled([ CallHistory.insertOne({ ...sharedData, uid: call.caller.id, direction: 'outbound', contactId: call.callee.id }), CallHistory.insertOne({ ...sharedData, uid: call.callee.id, direction: 'inbound', contactId: call.caller.id }), ]); + results.forEach((r, i) => { + if (r.status === 'rejected') { + logger.warn({ msg: 'failed to insert call history', callId: call._id, leg: i === 0 ? 'caller' : 'callee', error: r.reason }); + } + });
128-136: Clamp negative durations defensively.Clock skew or unexpected timestamps could yield negative diffs. Clamp to >= 0.
- const diff = endedAt.valueOf() - activatedAt.valueOf(); - return Math.floor(diff / 1000); + const diffMs = Math.max(0, endedAt.valueOf() - activatedAt.valueOf()); + return Math.floor(diffMs / 1000);
138-160: Make hangupReason check case‑insensitive and more robust.hangupReason shapes vary; consider a lowercase check and include common failure tokens.
- const hasError = call.hangupReason?.includes('error'); + const reason = call.hangupReason?.toLowerCase() ?? ''; + const hasError = /error|failed|timeout|ice|disconnect/.test(reason);packages/core-typings/src/ICallHistoryItem.ts (1)
17-34: Core item shapes look good; consider exporting base interfaces later.ICallHistoryItem and IMediaCallHistoryItem are internal here. If you foresee external consumers (apps or EE), exporting them could help. Optional.
📜 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 (12)
apps/meteor/server/models.ts(2 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/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
🧬 Code graph analysis (8)
packages/models/src/models/CallHistory.ts (2)
packages/core-typings/src/ICallHistoryItem.ts (1)
CallHistoryItem(46-46)packages/model-typings/src/models/ICallHistoryModel.ts (1)
ICallHistoryModel(5-5)
apps/meteor/server/models.ts (1)
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(46-46)
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)
ee/packages/media-calls/src/server/MediaCallServer.ts (1)
ee/packages/media-calls/src/logger.ts (1)
logger(3-3)
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(36-41)CallHistoryItemState(5-15)packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)
packages/core-typings/src/ICallHistoryItem.ts (2)
packages/core-typings/src/IUser.ts (1)
IUser(186-255)packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)
⏰ 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: 📦 Build Packages
🔇 Additional comments (15)
packages/model-typings/src/index.ts (1)
89-89: LGTM!The export follows the established pattern and is correctly placed alphabetically.
packages/models/src/index.ts (2)
94-94: LGTM!The import is correctly placed alphabetically and follows the established pattern.
149-149: LGTM!The proxified CallHistory model export follows the established pattern and is correctly positioned alphabetically.
packages/models/src/modelClasses.ts (1)
81-81: LGTM!The re-export follows the established pattern for model classes.
packages/core-typings/src/index.ts (1)
150-150: LGTM!The export is correctly placed near other media call-related exports and follows the established pattern.
ee/packages/media-calls/src/server/MediaCallServer.ts (1)
68-72: LGTM!The implementation follows the established pattern of
reportCallUpdateand correctly emits thehistoryUpdateevent for consumption by history persistence logic.ee/packages/media-calls/src/definition/IMediaCallServer.ts (1)
10-10: LGTM!The interface additions for
historyUpdateevent andupdateCallHistorymethod are consistent with existing patterns and properly typed.Also applies to: 43-43
ee/packages/media-calls/src/server/CallDirector.ts (2)
6-6: LGTM!The import addition is necessary for the new history update functionality.
371-377: LGTM!The history update logic is correctly placed after the database operation and only triggers when the call actually ends (modifiedCount > 0). This defensive approach prevents duplicate history updates for already-ended calls.
apps/meteor/server/models.ts (2)
12-12: LGTM!The import is correctly placed alphabetically and follows the established pattern.
99-99: LGTM!The model registration follows the established pattern and correctly instantiates the CallHistoryRaw model with the database instance.
packages/model-typings/src/models/ICallHistoryModel.ts (1)
1-5: LGTM: public model typing is clean and aligned with models/raw usage.packages/core-typings/src/ICallHistoryItem.ts (3)
5-15: State taxonomy is clear and sufficient for MVP.
36-41: Internal media call item fits service usage (contactId, rid?).Matches persisted fields and optional rid from DM resolution. LGTM.
46-46: Alias to IInternalMediaCallHistoryItem keeps surface minimal.Good choice for now; revisit when external call history is added.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/meteor/server/services/media-call/service.ts (1)
168-179: Critical: DM creation can receive undefined usernames.This issue was flagged in a previous review and remains unresolved. Users without usernames will pass the length check but produce
undefinedentries in the array, potentially breakingcreateDirectMessage.Validate both the array length and that all usernames are defined:
- const usernames = (await Users.findByIds(call.uids, { projection: { username: 1 } }).toArray()).map(({ username }) => username); - if (usernames.length !== 2) { - throw new Error('Invalid usernames for DM.'); - } + const userDocs = await Users.findByIds(call.uids, { projection: { username: 1 } }).toArray(); + if (userDocs.length !== 2 || userDocs.some((u) => !u.username)) { + throw new Error('Cannot create DM: missing usernames for one or both users.'); + } + const usernames = userDocs.map((u) => u.username as string);
🧹 Nitpick comments (2)
apps/meteor/server/services/media-call/service.ts (2)
95-95: Add logging when room resolution fails.Failed room lookups/creation are silently suppressed, making it difficult to diagnose DM creation issues.
Apply this diff to log the error:
- const rid = await this.getRoomIdForInternalCall(call).catch(() => undefined); + const rid = await this.getRoomIdForInternalCall(call).catch((error) => { + logger.warn({ msg: 'Failed to resolve room for call history', callId: call._id, error }); + return undefined; + });
110-123: Log failed history insertions.
Promise.allSettledallows partial failures to be ignored. If one insertion fails (e.g., database error), it won't be visible in logs.Apply this diff to log failures:
- await Promise.allSettled([ + const results = await Promise.allSettled([ CallHistory.insertOne({ ...sharedData, uid: call.caller.id, direction: 'outbound', contactId: call.callee.id, }), CallHistory.insertOne({ ...sharedData, uid: call.callee.id, direction: 'inbound', contactId: call.caller.id, }), ]); + + results.forEach((result, index) => { + if (result.status === 'rejected') { + logger.error({ msg: 'Failed to insert call history', callId: call._id, direction: index === 0 ? 'outbound' : 'inbound', error: result.reason }); + } + });
📜 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 (7)
📓 Common learnings
Learnt from: pierre-lehnen-rc
PR: RocketChat/Rocket.Chat#36718
File: packages/media-signaling/src/lib/Call.ts:633-642
Timestamp: 2025-09-23T00:27:05.438Z
Learning: In PR #36718, pierre-lehnen-rc prefers to maintain consistency with the old architecture patterns for DTMF handling rather than implementing immediate validation improvements, deferring enhancements to future work.
📚 Learning: 2025-09-16T13:33:49.237Z
Learnt from: cardoso
PR: RocketChat/Rocket.Chat#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
PR: RocketChat/Rocket.Chat#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
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
PR: RocketChat/Rocket.Chat#37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).
Applied to files:
apps/meteor/server/services/media-call/service.ts
📚 Learning: 2025-10-28T16:53:42.761Z
Learnt from: ricardogarim
PR: RocketChat/Rocket.Chat#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/server/services/media-call/service.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
PR: RocketChat/Rocket.Chat#37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings (mapping subscription documents to room IDs), never undefined, even when user has no room subscriptions.
Applied to files:
apps/meteor/server/services/media-call/service.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
PR: RocketChat/Rocket.Chat#37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings by mapping subscription documents to room IDs, never undefined, even when user has no room subscriptions.
Applied to files:
apps/meteor/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)packages/models/src/index.ts (3)
CallHistory(149-149)Rooms(204-204)Users(213-213)packages/core-typings/src/ICallHistoryItem.ts (2)
IInternalMediaCallHistoryItem(36-41)CallHistoryItemState(5-15)
⏰ 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/server/services/media-call/service.ts (4)
2-2: LGTM: Imports are appropriate for the new functionality.All new imports are properly used in the call history implementation.
Also applies to: 6-7, 10-10
68-87: LGTM: Validation logic correctly guards history persistence.The method properly validates call existence, completion status, and filters for internal calls before persisting.
128-136: LGTM: Duration calculation is correct.The logic properly handles unactivated calls and converts milliseconds to seconds.
138-160: LGTM: State derivation logic correctly prioritizes conditions.The method appropriately distinguishes between transferred, failed, error, not-answered, and ended states based on call lifecycle flags.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/meteor/server/services/media-call/service.ts (1)
21-21: Catch saveCallToHistory failures
saveCallToHistoryis async and any rejection will bubble out ofsetImmediateas an unhandled promise rejection, risking process termination in modern Node and hiding call-history persistence failures. Please catch and log the error (similar to the comment already raised on this line) so we know when a history write fails.- callServer.emitter.on('historyUpdate', ({ callId }) => setImmediate(() => this.saveCallToHistory(callId))); + callServer.emitter.on('historyUpdate', ({ callId }) => + setImmediate(() => + this.saveCallToHistory(callId).catch((error) => { + logger.error({ msg: 'Failed to save call to history', callId, error }); + }), + ), + );
📜 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 (7)
📓 Common learnings
Learnt from: pierre-lehnen-rc
PR: RocketChat/Rocket.Chat#36718
File: packages/media-signaling/src/lib/Call.ts:633-642
Timestamp: 2025-09-23T00:27:05.438Z
Learning: In PR #36718, pierre-lehnen-rc prefers to maintain consistency with the old architecture patterns for DTMF handling rather than implementing immediate validation improvements, deferring enhancements to future work.
📚 Learning: 2025-09-16T13:33:49.237Z
Learnt from: cardoso
PR: RocketChat/Rocket.Chat#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
PR: RocketChat/Rocket.Chat#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
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
PR: RocketChat/Rocket.Chat#37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).
Applied to files:
apps/meteor/server/services/media-call/service.ts
📚 Learning: 2025-10-28T16:53:42.761Z
Learnt from: ricardogarim
PR: RocketChat/Rocket.Chat#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/server/services/media-call/service.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
PR: RocketChat/Rocket.Chat#37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings (mapping subscription documents to room IDs), never undefined, even when user has no room subscriptions.
Applied to files:
apps/meteor/server/services/media-call/service.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
PR: RocketChat/Rocket.Chat#37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings by mapping subscription documents to room IDs, never undefined, even when user has no room subscriptions.
Applied to files:
apps/meteor/server/services/media-call/service.ts
🧬 Code graph analysis (1)
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(36-41)CallHistoryItemState(5-15)packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)
⏰ 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
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/meteor/server/services/media-call/service.ts (1)
113-126: Insertion failures are logged but not surfaced to callers.Each
insertOnecall now has a.catch()handler that logs errors without rethrowing. This means:
- The promises resolve (not reject) even on database errors
Promise.allSettledalways sees fulfilled promisessaveInternalCallToHistorycompletes successfully even if both insertions failed- VGA-50's requirement to "persist Call History entry" can silently fail
Per the past review and VGA-50 objectives, database insertion failures should propagate so callers know the operation failed.
Consider this approach:
- await Promise.allSettled([ + const results = await Promise.allSettled([ CallHistory.insertOne({ ...sharedData, uid: call.caller.id, direction: 'outbound', contactId: call.callee.id, - }).catch((error: unknown) => logger.error({ msg: 'Failed to insert item into Call History', error })), + }), CallHistory.insertOne({ ...sharedData, uid: call.callee.id, direction: 'inbound', contactId: call.caller.id, - }).catch((error: unknown) => logger.error({ msg: 'Failed to insert item into Call History', error })), + }), ]); + + const failures = results.filter((r) => r.status === 'rejected'); + if (failures.length > 0) { + failures.forEach((f) => logger.error({ msg: 'Failed to insert item into Call History', callId: call._id, error: f.reason })); + throw new Error(`Failed to insert ${failures.length} call history record(s)`); + }
🧹 Nitpick comments (2)
apps/meteor/server/services/media-call/service.ts (2)
176-182: Consider more explicit username validation for type safety.The current implementation filters falsy usernames and then validates the count, which works correctly at runtime. However, for better type safety and clarity, consider validating the user documents before mapping, as suggested in the past review:
- const usernames = (await Users.findByIds(call.uids, { projection: { username: 1 } }).toArray()) - .map(({ username }) => username) - .filter((username) => username); - - if (usernames.length !== 2) { - throw new Error('Invalid usernames for DM.'); - } + const userDocs = await Users.findByIds(call.uids, { projection: { username: 1 } }).toArray(); + if (userDocs.length !== 2 || userDocs.some((u) => !u.username)) { + throw new Error('Cannot create DM: missing usernames for one or both users.'); + } + const usernames = userDocs.map((u) => u.username as string);This approach:
- Makes the validation logic more explicit
- Improves TypeScript type narrowing (
username as stringis safe after validation)- Provides a clearer error message distinguishing between missing users and missing usernames
141-163: Consider typinghangupReasonexplicitly or use a set-based check for error detection.Line 50 in
packages/core-typings/src/mediaCalls/IMediaCall.tstypeshangupReasonasstringrather than the strictCallHangupReasonunion. While the substring matchincludes('error')correctly identifies all current error reasons ('signaling-error','service-error','media-error','input-error'), this approach relies on naming conventions rather than type enforcement.To improve type safety and maintainability, consider either:
- Typing
hangupReasonasCallHangupReasoninstead ofstring- Using an explicit set-based check:
['signaling-error', 'service-error', 'media-error', 'input-error'].includes(call.hangupReason)
📜 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 (7)
📓 Common learnings
Learnt from: pierre-lehnen-rc
Repo: RocketChat/Rocket.Chat PR: 36718
File: packages/media-signaling/src/lib/Call.ts:633-642
Timestamp: 2025-09-23T00:27:05.438Z
Learning: In PR #36718, pierre-lehnen-rc prefers to maintain consistency with the old architecture patterns for DTMF handling rather than implementing immediate validation improvements, deferring enhancements to future work.
📚 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
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).
Applied to files:
apps/meteor/server/services/media-call/service.ts
📚 Learning: 2025-10-28T16:53:42.761Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Applied to files:
apps/meteor/server/services/media-call/service.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings (mapping subscription documents to room IDs), never undefined, even when user has no room subscriptions.
Applied to files:
apps/meteor/server/services/media-call/service.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings by mapping subscription documents to room IDs, never undefined, even when user has no room subscriptions.
Applied to files:
apps/meteor/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)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(36-41)CallHistoryItemState(5-15)
⏰ 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/server/services/media-call/service.ts (4)
68-87: LGTM: Call validation and routing logic is clear.The method correctly validates that the call exists, has ended, and is an internal call (2 participants). The early returns for invalid scenarios are appropriate, and the TODO comment clearly indicates that external call support is planned for future work.
95-98: Verify: Should DM resolution failures allow history persistence without room ID?The current code logs DM resolution errors but continues with
ridasundefined. While line 110 makesridoptional in the history record, VGA-53's objective is to "identify the DM room for the two call participants and create that DM room if it does not yet exist."If creating or finding the DM fails, should we:
- Continue persisting call history without
rid(current behavior)- Fail the entire history operation and propagate the error
Please clarify if persisting call history records without a valid
ridis acceptable for internal calls, or if DM resolution is a hard requirement.
131-139: LGTM: Duration calculation is correct.The method correctly computes call duration in seconds from
activatedAttoendedAt, returning 0 for calls that never activated (never connected). The fallback tonew Date()is defensive and appropriate.
171-174: LGTM: DM creator selection logic is robust.The fallback chain (
requesterId || callerId || call.uids[0]) appropriately handles various scenarios, ensuring a valid user ID is selected to create the DM even whencreatedByis not a user-type contact.
tassoevan
added
the
stat: QA assured
Proposed changes (including videos or screenshots)
When an internal call ends, we now create a record on a per-user call history and detect the room id of the DM between the two users (creating it if it doesn't exist), so that we may send a message with the call's status on that room.
Issue(s)
VGA-49
VGA-50
VGA-53
Steps to test or reproduce
Further comments
The Call History itself will be expanded with API endpoints and a UI, as well as covering other types of calls too - but that work will be done with a different milestone; this PR simply adds the minimum history features needed to send the call messages on DMs.
Summary by CodeRabbit
New Features
Chores