feat: allow clients to accept calls before the session is established#38646
feat: allow clients to accept calls before the session is established#38646pierre-lehnen-rc wants to merge 8 commits intodevelopfrom
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 |
🦋 Changeset detectedLatest commit: bb0aed3 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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a POST REST endpoint to answer media calls, implements MediaCallService.answerCall, introduces SignalProcessingOptions with throwIfSkipped, threads async signal processing through MediaCallServer and SignalProcessors, updates models/typings for callee-scoped lookups, and adjusts signaling enums/types. Changes
Sequence DiagramsequenceDiagram
participant Client as REST Client
participant API as API Endpoint
participant Service as MediaCallService
participant CallServer as MediaCallServer
participant Processor as SignalProcessor
Client->>API: POST /api/v1/media-calls.answer (body)
API->>Service: answerCall(uid, params)
Service->>CallServer: receiveSignal(fromUid, signal, {throwIfSkipped: true})
CallServer->>Processor: processSignal(uid, signal, {throwIfSkipped: true})
Processor->>Processor: validate call, actor, contract
alt Contract mismatch & throwIfSkipped
Processor-->>Service: throw invalid-contract
else Process signal successfully
Processor-->>CallServer: resolve
end
CallServer-->>Service: resolve
Service->>Service: reloadCall(callId) & verify contractId
Service-->>API: IMediaCall
API-->>Client: 200 OK (IMediaCall)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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 Tip CodeRabbit can use your project's `biome` configuration to improve the quality of JS/TS/CSS/JSON code reviews.Add a configuration file to your project to customize how CodeRabbit runs |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #38646 +/- ##
===========================================
- Coverage 70.93% 70.88% -0.05%
===========================================
Files 3209 3209
Lines 113875 113910 +35
Branches 20618 20648 +30
===========================================
- Hits 80772 80747 -25
- Misses 31054 31109 +55
- Partials 2049 2054 +5
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/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 19:01", "12/30 20:52", "02/12 22:57", "02/13 22:38", "02/16 14:04", "02/16 19:37 (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.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, 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.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, 0.13, 0.13, 0.13, 0.13]
line "rocketchat" [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.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, 16 Feb 2026 19:37:42 GMT |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/meteor/app/api/server/v1/media-calls.ts`:
- Around line 58-76: The response schema compiled via ajv.compile that defines
the 200 response currently marks the "call" property as nullable; update that
schema (the object passed into ajv.compile in
apps/meteor/app/api/server/v1/media-calls.ts) by removing "nullable: true" from
the "call" property so it reflects the actual behavior of answerCall (which
always returns a non-null IMediaCall or throws); keep the rest of the schema
(required: ['call','success'], $ref to IMediaCall) unchanged.
In `@apps/meteor/server/services/media-call/service.ts`:
- Around line 43-73: In answerCall, after confirming the callee via
MediaCalls.findOneById and before invoking callServer.receiveSignal, perform an
explicit permission check that verifies the user (uid) is allowed to answer
media calls (e.g., the specific permission key like
"allow-internal-voice-calls"); if the check fails, throw an appropriate error
(e.g., 'access-denied' or 'not-allowed'). Locate the answerCall method and add
the permission check using your project's existing permission utility (or
permissionCheck helper) for the uid and the contract/context from updated call
metadata, then only call callServer.receiveSignal when the permission check
passes.
🧹 Nitpick comments (4)
apps/meteor/server/services/media-call/service.ts (2)
46-48: Unused projection fieldacceptedAt.The
acceptedAtfield is included in the projection but never referenced in the method logic. Either remove it or use it for an early check (e.g., rejecting already-accepted calls before sending the signal).Proposed fix
- const call = await MediaCalls.findOneById<Pick<IMediaCall, '_id' | 'callee' | 'acceptedAt'>>(callId, { - projection: { callee: 1, acceptedAt: 1 }, + const call = await MediaCalls.findOneById<Pick<IMediaCall, '_id' | 'callee'>>(callId, { + projection: { callee: 1 }, });
65-70: Clarify the contractId validation logic.The nested conditional handles two distinct failure modes but both paths surface as opaque error strings. If
updatedCall.callee.contractIdexists but doesn't matchsignal.contractId, it means another client session answered first (invalid-call-state). IfcontractIdis falsy after processing, it's an unexpected server-side failure (internal-error). This is reasonable, but consider whether the caller (REST endpoint) can meaningfully distinguish these — both will likely surface as 500s without error-class mapping (as noted in the endpoint review).ee/packages/media-calls/src/internal/agents/CallSignalProcessor.ts (1)
62-73:throwIfSkippedstored as mutable instance state is fragile.Setting
throwIfSkippedas instance-level state inprocessSignal(line 97) and reading it later invalidatePendingCalleecreates implicit coupling and would break under concurrent signal processing on the same instance. While this may not be an issue today given the processing model, passingoptions(or the flag) as a parameter through the call chain would be more robust and explicit.Sketch: thread options through methods instead of instance state
- private throwIfSkipped: boolean; ... - this.throwIfSkipped = false; ... - this.throwIfSkipped = options.throwIfSkipped || false; ... - protected validatePendingCallee(): boolean { + protected validatePendingCallee(throwIfSkipped = false): boolean { if (this.role !== 'callee') { - if (this.throwIfSkipped) { + if (throwIfSkipped) { throw new Error('invalid-call-role'); } return false; } if (!this.isCallPending()) { - if (this.throwIfSkipped) { + if (throwIfSkipped) { throw new Error('invalid-call-state'); } return false; } return true; }Then pass the flag through
processAnswer→clientHasAccepted/clientHasRejected→validatePendingCallee.Also applies to: 87-97
ee/packages/media-calls/src/internal/SignalProcessor.ts (1)
105-120: Minor: double logging on actor-readiness failure.When the
UserActorAgentcheck fails, the error is logged with full context at Lines 106–112, then the thrownError('internal-error')is caught at Line 117 and logged again at Line 118 ({ err: e }). This produces two log entries for the same failure.Not a bug—the catch block is a shared safety net for all error paths in this method, and the contextual log is valuable. Just something to be aware of if log noise becomes a concern.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/meteor/app/api/server/index.tsapps/meteor/app/api/server/v1/media-calls.tsapps/meteor/server/services/media-call/service.tsee/packages/media-calls/src/definition/IMediaCallServer.tsee/packages/media-calls/src/definition/common.tsee/packages/media-calls/src/internal/SignalProcessor.tsee/packages/media-calls/src/internal/agents/CallSignalProcessor.tsee/packages/media-calls/src/server/MediaCallServer.tspackages/core-services/src/types/IMediaCallService.tspackages/media-signaling/src/definition/call/IClientMediaCall.tspackages/media-signaling/src/definition/signals/client/answer.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:
ee/packages/media-calls/src/definition/common.tspackages/core-services/src/types/IMediaCallService.tsapps/meteor/app/api/server/index.tsapps/meteor/app/api/server/v1/media-calls.tsee/packages/media-calls/src/definition/IMediaCallServer.tsee/packages/media-calls/src/internal/SignalProcessor.tspackages/media-signaling/src/definition/signals/client/answer.tsee/packages/media-calls/src/server/MediaCallServer.tspackages/media-signaling/src/definition/call/IClientMediaCall.tsee/packages/media-calls/src/internal/agents/CallSignalProcessor.tsapps/meteor/server/services/media-call/service.ts
🧠 Learnings (4)
📓 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-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:
packages/core-services/src/types/IMediaCallService.tsapps/meteor/app/api/server/index.tsapps/meteor/app/api/server/v1/media-calls.tsee/packages/media-calls/src/definition/IMediaCallServer.tsapps/meteor/server/services/media-call/service.ts
📚 Learning: 2025-12-18T15:18:31.688Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37773
File: apps/meteor/client/views/mediaCallHistory/MediaCallHistoryInternal.tsx:24-34
Timestamp: 2025-12-18T15:18:31.688Z
Learning: In apps/meteor/client/views/mediaCallHistory/MediaCallHistoryInternal.tsx, for internal call history items, the item.contactId is guaranteed to always match either the caller.id or callee.id in the call data, so the contact resolution in getContact will never result in undefined.
Applied to files:
packages/core-services/src/types/IMediaCallService.tsee/packages/media-calls/src/definition/IMediaCallServer.tsee/packages/media-calls/src/server/MediaCallServer.tsapps/meteor/server/services/media-call/service.ts
📚 Learning: 2025-11-19T18:20:07.720Z
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.720Z
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/service.ts
🧬 Code graph analysis (8)
packages/core-services/src/types/IMediaCallService.ts (2)
packages/media-signaling/src/definition/signals/client/answer.ts (1)
ClientMediaSignalAnswer(7-15)packages/core-typings/src/mediaCalls/IMediaCall.ts (1)
IMediaCall(35-71)
apps/meteor/app/api/server/v1/media-calls.ts (5)
packages/media-signaling/src/definition/call/IClientMediaCall.ts (4)
CallAnswer(60-60)CallFeature(23-23)callAnswerList(53-58)callFeatureList(21-21)packages/rest-typings/src/v1/Ajv.ts (5)
ajv(24-24)validateBadRequestErrorResponse(47-47)validateUnauthorizedErrorResponse(70-70)validateForbiddenErrorResponse(93-93)validateNotFoundErrorResponse(109-109)apps/meteor/app/api/server/index.ts (1)
API(53-53)packages/core-typings/src/mediaCalls/IMediaCall.ts (1)
IMediaCall(35-71)apps/meteor/app/api/server/ApiClass.ts (1)
ExtractRoutesFromAPI(74-78)
ee/packages/media-calls/src/definition/IMediaCallServer.ts (1)
ee/packages/media-calls/src/definition/common.ts (1)
SignalProcessingOptions(32-36)
ee/packages/media-calls/src/internal/SignalProcessor.ts (1)
ee/packages/media-calls/src/definition/common.ts (1)
SignalProcessingOptions(32-36)
packages/media-signaling/src/definition/signals/client/answer.ts (1)
packages/media-signaling/src/definition/call/IClientMediaCall.ts (1)
callAnswerList(53-58)
ee/packages/media-calls/src/server/MediaCallServer.ts (1)
ee/packages/media-calls/src/definition/common.ts (1)
SignalProcessingOptions(32-36)
ee/packages/media-calls/src/internal/agents/CallSignalProcessor.ts (2)
ee/packages/media-calls/src/definition/common.ts (1)
SignalProcessingOptions(32-36)ee/packages/media-calls/src/server/CallDirector.ts (1)
mediaCallDirector(449-449)
apps/meteor/server/services/media-call/service.ts (4)
packages/media-signaling/src/definition/signals/client/answer.ts (1)
ClientMediaSignalAnswer(7-15)packages/core-typings/src/mediaCalls/IMediaCall.ts (1)
IMediaCall(35-71)ee/packages/media-calls/src/internal/agents/CallSignalProcessor.ts (1)
callId(34-36)packages/media-signaling/src/lib/Call.ts (1)
callId(57-59)
⏰ 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 (10)
packages/media-signaling/src/definition/call/IClientMediaCall.ts (1)
53-60: Clean extraction of runtime-validated answer values.The
callAnswerList+CallAnswerpattern mirrors the existingcallFeatureList/CallFeaturepattern on lines 21–23, keeping the file consistent and enabling shared runtime validation in JSON schemas.packages/media-signaling/src/definition/signals/client/answer.ts (1)
4-4: Good DRY improvement — single source of truth for answer enum values.Replacing the hard-coded literal array with the imported
callAnswerListensures the schema stays in sync with the type definition automatically.Also applies to: 36-36
packages/core-services/src/types/IMediaCallService.ts (1)
1-9: LGTM!Interface addition is clean and consistent with the implementation in
service.ts.apps/meteor/app/api/server/index.ts (1)
25-25: LGTM!Import is correctly placed in alphabetical order and before OpenAPI generation.
ee/packages/media-calls/src/definition/common.ts (1)
32-36: LGTM!Clean type addition with clear documentation of the
throwIfSkippedflag's purpose.ee/packages/media-calls/src/server/MediaCallServer.ts (1)
44-46: LGTM!Clean delegation with options propagation. Signal validation is appropriately handled at the entry points (REST schema validation and
deserializeClientSignal).ee/packages/media-calls/src/definition/IMediaCallServer.ts (1)
44-44: LGTM!Interface signature is consistent with the implementation in
MediaCallServer.ts.apps/meteor/app/api/server/v1/media-calls.ts (1)
85-91: Please provide the review comment that needs to be rewritten and verified.ee/packages/media-calls/src/internal/SignalProcessor.ts (2)
93-98:throwIfSkippedguard for contract mismatch looks correct.The branching logic properly differentiates between WebSocket-style processing (silently skip mismatched contracts) and REST-style processing (throw to surface the error). The
undefinedcase forthrowIfSkippedcorrectly falls through to the silent return.
13-13: Clean threading ofSignalProcessingOptionsthrough the processing pipeline.The
optionsparameter is consistently passed fromprocessSignaldown toprocessCallSignal, and correctly not forwarded toprocessRegisterSignal/processRequestCallSignalsince those paths don't need contract-check behavior. The destructuring of{ throwIfSkipped }is appropriate given the optional field in the type.Note:
optionsis a required parameter on the publicprocessSignalmethod. All existing callers have been properly updated—MediaCallServer.ts already passes the options argument correctly.Likely an incorrect or invalid review comment.
![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 11 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/app/api/server/v1/media-calls.ts">
<violation number="1" location="apps/meteor/app/api/server/v1/media-calls.ts:45">
P3: The `nullable: true` on the `call` property is inconsistent with the actual return type. The `answerCall()` method always returns an `IMediaCall` or throws an error - it never returns null. Remove `nullable: true` to accurately reflect the API contract.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/model-typings/src/models/IMediaCallsModel.ts">
<violation number="1" location="packages/model-typings/src/models/IMediaCallsModel.ts:17">
P3: `findOneByIdAndCallee` is generic over `T` but the `options` parameter is fixed to `FindOptions<IMediaCall>`, which blocks callers from using `FindOptions<T>` when projecting to another type. Align it with the other generic methods to keep typings consistent.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
coderabbitai
bot
added
the
type: feature
dionisio-bot
bot
removed
the
type: feature
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/server/services/media-call/service.ts (1)
60-65: Potential race condition between signal processing and call reload.After
receiveSignalcompletes at line 60, there's a window beforefindOneByIdat line 62 where another operation could modify or delete the call. While unlikely in normal operation, consider using a transaction or returning the updated call directly fromreceiveSignalif the underlying implementation supports it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/services/media-call/service.ts` around lines 60 - 65, There’s a race between callServer.receiveSignal(...) and the subsequent MediaCalls.findOneById(callId); to fix, change the implementation so receiveSignal returns the updated MediaCall object (or null) and use that returned value instead of calling MediaCalls.findOneById, or, if changing receiveSignal isn’t possible, perform the read inside the same DB transaction used by receiveSignal or add a short retry/read-verify loop around MediaCalls.findOneById(callId) to handle transient missing/modified records; update callers of callServer.receiveSignal and the code that currently uses MediaCalls.findOneById to rely on the returned updated call (or handle the retry/transaction result) accordingly, referencing callServer.receiveSignal and MediaCalls.findOneById.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/meteor/server/services/media-call/service.ts`:
- Around line 60-65: There’s a race between callServer.receiveSignal(...) and
the subsequent MediaCalls.findOneById(callId); to fix, change the implementation
so receiveSignal returns the updated MediaCall object (or null) and use that
returned value instead of calling MediaCalls.findOneById, or, if changing
receiveSignal isn’t possible, perform the read inside the same DB transaction
used by receiveSignal or add a short retry/read-verify loop around
MediaCalls.findOneById(callId) to handle transient missing/modified records;
update callers of callServer.receiveSignal and the code that currently uses
MediaCalls.findOneById to rely on the returned updated call (or handle the
retry/transaction result) accordingly, referencing callServer.receiveSignal and
MediaCalls.findOneById.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.changeset/fair-lions-smell.mdapps/meteor/server/services/media-call/service.tspackages/model-typings/src/models/IMediaCallsModel.tspackages/models/src/models/MediaCalls.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/models/src/models/MediaCalls.tspackages/model-typings/src/models/IMediaCallsModel.tsapps/meteor/server/services/media-call/service.ts
🧠 Learnings (8)
📓 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-12-18T15:18:31.688Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37773
File: apps/meteor/client/views/mediaCallHistory/MediaCallHistoryInternal.tsx:24-34
Timestamp: 2025-12-18T15:18:31.688Z
Learning: In apps/meteor/client/views/mediaCallHistory/MediaCallHistoryInternal.tsx, for internal call history items, the item.contactId is guaranteed to always match either the caller.id or callee.id in the call data, so the contact resolution in getContact will never result in undefined.
Applied to files:
packages/models/src/models/MediaCalls.tspackages/model-typings/src/models/IMediaCallsModel.tsapps/meteor/server/services/media-call/service.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:
packages/models/src/models/MediaCalls.tspackages/model-typings/src/models/IMediaCallsModel.tsapps/meteor/server/services/media-call/service.ts
📚 Learning: 2026-02-24T19:09:09.561Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Applied to files:
.changeset/fair-lions-smell.md
📚 Learning: 2026-02-24T19:05:56.710Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
Applied to files:
.changeset/fair-lions-smell.md
📚 Learning: 2025-11-19T18:20:07.720Z
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.720Z
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:
.changeset/fair-lions-smell.mdapps/meteor/server/services/media-call/service.ts
📚 Learning: 2025-11-04T16:49:19.107Z
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.
Applied to files:
apps/meteor/server/services/media-call/service.ts
📚 Learning: 2026-02-25T20:10:16.987Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38913
File: packages/ddp-client/src/legacy/types/SDKLegacy.ts:34-34
Timestamp: 2026-02-25T20:10:16.987Z
Learning: In the RocketChat/Rocket.Chat monorepo, packages/ddp-client and apps/meteor do not use TypeScript project references. Module augmentations in apps/meteor (e.g., declare module 'rocket.chat/rest-typings') are not visible when compiling packages/ddp-client in isolation, which is why legacy SDK methods that depend on OperationResult types for OpenAPI-migrated endpoints must remain commented out.
Applied to files:
apps/meteor/server/services/media-call/service.ts
🧬 Code graph analysis (3)
packages/models/src/models/MediaCalls.ts (1)
packages/core-typings/src/mediaCalls/IMediaCall.ts (2)
IMediaCall(35-71)MediaCallActor(7-11)
packages/model-typings/src/models/IMediaCallsModel.ts (1)
packages/core-typings/src/mediaCalls/IMediaCall.ts (2)
IMediaCall(35-71)MediaCallActor(7-11)
apps/meteor/server/services/media-call/service.ts (3)
packages/media-signaling/src/definition/signals/client/answer.ts (1)
ClientMediaSignalAnswer(7-15)ee/packages/media-calls/src/internal/agents/CallSignalProcessor.ts (1)
callId(34-36)packages/media-signaling/src/lib/Call.ts (1)
callId(57-59)
🔇 Additional comments (7)
.changeset/fair-lions-smell.md (1)
1-10: LGTM!The changeset correctly identifies all affected packages with appropriate minor version bumps for this new feature. The description accurately summarizes the REST endpoint addition for accepting/rejecting media calls without an active session.
packages/model-typings/src/models/IMediaCallsModel.ts (1)
14-18: LGTM!The new
findOneByIdAndCalleemethod signature is well-typed and follows the existing patterns in the interface. It correctly usesMediaCallActorto enable callee-scoped lookups, supporting the new REST endpoint for answering calls.packages/models/src/models/MediaCalls.ts (1)
41-55: LGTM!The implementation is clean and correct:
- Query leverages the always-indexed
_idfield for efficient lookup- Additional callee field filters (
type,id) ensure proper authorization- The conditional
contractIdfilter using spread is a clean pattern for optional matching- Method correctly implements the
IMediaCallsModelinterface signatureapps/meteor/server/services/media-call/service.ts (4)
12-13: LGTM!Proper separation of type-only imports from runtime imports. This is idiomatic TypeScript and aids tree-shaking.
43-54: Consider adding explicit permission check for defense-in-depth.The method validates the user is the callee but relies on
callServer's configuredpermissionCheckfor permission enforcement. For consistency with other service methods and defense-in-depth, consider adding an explicituserHasMediaCallPermissioncheck before processing the signal.
77-83: LGTM!Proper async/await usage for
receiveSignal. This ensures errors are caught correctly in the try-catch block.
85-96: LGTM!Good enhancement capturing
signalTypefor enriched error logging. Declaring the variable before the try block ensures it's available in the catch block for diagnostic purposes.
pierre-lehnen-rc
added
the
stat: QA assured
coderabbitai
bot
added
the
type: feature
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/server/services/media-call/service.ts (1)
67-83: Consider pre-validating call state before signal processing.The switch validation occurs after
callServer.receiveSignalhas already processed the signal. If these checks fail (e.g.,acceptedAtis set for anacksignal), the signal was still processed but an error is returned to the client, potentially leaving state inconsistent.For the
ackcase specifically (lines 69-71), consider checkingacceptedAtandendedbefore callingreceiveSignal, or extending the initial query at lines 46-50 to include these fields and validate upfront.♻️ Suggested approach: Pre-validation with extended query
- const call = await MediaCalls.findOneByIdAndCallee<Pick<IMediaCall, '_id'>>( + const call = await MediaCalls.findOneByIdAndCallee<Pick<IMediaCall, '_id' | 'acceptedAt' | 'ended' | 'callee'>>( callId, { type: 'user', id: uid }, - { projection: { _id: 1 } }, + { projection: { _id: 1, acceptedAt: 1, ended: 1, callee: 1 } }, ); if (!call) { throw new Error('not-found'); } + + if (answer === 'ack' && (call.acceptedAt || call.ended)) { + throw new Error('invalid-call-state'); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/services/media-call/service.ts` around lines 67 - 83, Pre-validate the call state before invoking callServer.receiveSignal: extend the initial call lookup used earlier to include updatedCall.acceptedAt, updatedCall.ended and updatedCall.callee.contractId (or fetch the same fields) and perform the switch checks (answer === 'ack' -> ensure !acceptedAt && !ended; answer === default -> ensure callee.contractId === signal.contractId or throw appropriate errors) before calling callServer.receiveSignal; if validation fails, return/throw immediately so receiveSignal is never called with an invalid state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/meteor/server/services/media-call/service.ts`:
- Around line 67-83: Pre-validate the call state before invoking
callServer.receiveSignal: extend the initial call lookup used earlier to include
updatedCall.acceptedAt, updatedCall.ended and updatedCall.callee.contractId (or
fetch the same fields) and perform the switch checks (answer === 'ack' -> ensure
!acceptedAt && !ended; answer === default -> ensure callee.contractId ===
signal.contractId or throw appropriate errors) before calling
callServer.receiveSignal; if validation fails, return/throw immediately so
receiveSignal is never called with an invalid state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0fb54a16-f44d-4c7b-8af9-10921bb1682d
📒 Files selected for processing (1)
apps/meteor/server/services/media-call/service.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/server/services/media-call/service.ts
🧠 Learnings (8)
📓 Common learnings
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:50.283Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
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-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/server/services/media-call/service.ts
📚 Learning: 2025-11-04T16:49:19.107Z
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.
Applied to files:
apps/meteor/server/services/media-call/service.ts
📚 Learning: 2025-11-19T18:20:07.720Z
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.720Z
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/service.ts
📚 Learning: 2026-02-25T20:10:16.987Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38913
File: packages/ddp-client/src/legacy/types/SDKLegacy.ts:34-34
Timestamp: 2026-02-25T20:10:16.987Z
Learning: In the RocketChat/Rocket.Chat monorepo, packages/ddp-client and apps/meteor do not use TypeScript project references. Module augmentations in apps/meteor (e.g., declare module 'rocket.chat/rest-typings') are not visible when compiling packages/ddp-client in isolation, which is why legacy SDK methods that depend on OperationResult types for OpenAPI-migrated endpoints must remain commented out.
Applied to files:
apps/meteor/server/services/media-call/service.ts
📚 Learning: 2025-12-18T15:18:31.688Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37773
File: apps/meteor/client/views/mediaCallHistory/MediaCallHistoryInternal.tsx:24-34
Timestamp: 2025-12-18T15:18:31.688Z
Learning: In apps/meteor/client/views/mediaCallHistory/MediaCallHistoryInternal.tsx, for internal call history items, the item.contactId is guaranteed to always match either the caller.id or callee.id in the call data, so the contact resolution in getContact will never result in undefined.
Applied to files:
apps/meteor/server/services/media-call/service.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/server/services/media-call/service.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/server/services/media-call/service.ts
🔇 Additional comments (4)
apps/meteor/server/services/media-call/service.ts (4)
12-13: LGTM!Clean separation of type-only imports from the runtime
isClientMediaSignalvalidation function.
43-86: Add explicit media call permission check in answerCall.This concern was previously raised: the method validates that the user is the callee but does not check media call permissions (e.g.,
allow-internal-voice-calls). WhilepermissionCheckis configured forcallServer, adding an explicit check here would provide defense in depth.
88-94: LGTM!Properly awaiting
callServer.receiveSignalensures errors are caught and logged correctly.
96-107: LGTM!Good improvement to error diagnostics by capturing and logging the signal type. The
nullfallback correctly indicates deserialization failure scenarios.
There was a problem hiding this comment.
2 issues found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/server/services/media-call/service.ts">
<violation number="1" location="apps/meteor/server/services/media-call/service.ts:73">
P1: `unavailable` is reported as success even though unsigned clients are ignored, so push-notification declines can leave the call ringing.</violation>
<violation number="2" location="apps/meteor/server/services/media-call/service.ts:76">
P1: `reject` still requires a signed `callee.contractId` after processing, so rejecting from the new pre-session endpoint can fail with `internal-error`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
coderabbitai
bot
removed
the
type: feature
pierre-lehnen-rc
removed
the
stat: QA assured
3 participants
Proposed changes (including videos or screenshots)
Currently the call can only be accepted from a websocket connection, but in order to implement mobile support the app needs to be able to accept the call from a push notification, before a websocket connection is established. This PR adds an endpoint for that.
Issue(s)
VMUX-35
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Improvements