feat: send push notifications for voice calls#39386
feat: send push notifications for voice calls#39386pierre-lehnen-rc wants to merge 41 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 |
|
|
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 VoIP push support and wiring: voipToken and pushTokenTypes added to typings/models, push sending updated to support VoIP tokens and retry caps, token removal switched to string-based APIs, and media-call components emit and handle VoIP push notifications. Changes
Sequence DiagramsequenceDiagram
participant Caller
participant MediaCallServer
participant PushService
participant PushTokenDB
participant Gateway
Caller->>MediaCallServer: create/accept/end call (callId)
MediaCallServer->>MediaCallServer: emit pushNotificationRequest(callId, event)
MediaCallServer->>PushService: sendPushNotification(callId, event)
PushService->>PushTokenDB: findAllTokensByUserId / findTokensByUserIdExceptId(userId, skipTokenId)
PushTokenDB-->>PushService: tokens (may include voipToken)
PushService->>PushService: set retryOptions (tries/maxRetries = useVoipToken ? 1 : MAX)
alt useVoipToken & voipToken present
PushService->>Gateway: send APN VoIP (expiry 60s, pushType='voip')
else standard token
PushService->>Gateway: send APN/FCM with retries/backoff
end
Gateway-->>PushService: delivery result / error
PushService->>PushTokenDB: removeOrUnsetByTokenString(token) on invalid token
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 #39386 +/- ##
===========================================
- Coverage 70.21% 70.16% -0.06%
===========================================
Files 3280 3283 +3
Lines 116814 116889 +75
Branches 20715 20923 +208
===========================================
- Hits 82021 82015 -6
- Misses 31516 31600 +84
+ Partials 3277 3274 -3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
pierre-lehnen-rc
force-pushed
the
feat/voip-send-push
branch
2 times, most recently
from
04c0d47 to
1a94148
Compare
pierre-lehnen-rc
force-pushed
the
feat/voip-send-push
branch
from
1a94148 to
0eaeee8
Compare
pierre-lehnen-rc
force-pushed
the
feat/voip-send-push
branch
from
7a63dfd to
d990aca
Compare
coderabbitai
bot
added
the
type: feature
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found across 20 files
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/app/push/server/definition.ts">
<violation number="1" location="apps/meteor/app/push/server/definition.ts:45">
P1: Strip `useVoipToken` before building the gateway payload, otherwise VoIP notifications can start sending an unexpected field to the push gateway.</violation>
</file>
<file name="ee/packages/media-calls/src/definition/IMediaCallServer.ts">
<violation number="1" location="ee/packages/media-calls/src/definition/IMediaCallServer.ts:7">
P3: Remove this unused duplicate type alias; the same `VoipPushNotificationType` union already exists in `getPushNotificationType.ts`.
(Based on your team's feedback about removing unused types and configuration entries.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/meteor/app/push/server/apn.ts">
<violation number="1" location="apps/meteor/app/push/server/apn.ts:98">
P1: Avoid removing the whole push-token record when a VoIP token fails. Here `userToken` can be `app.voipToken`, and `removeAllByTokenString()` will delete the document that also stores the regular APN token.
(Based on your team's feedback about keeping VoIP tokens distinct from the paired regular token lifecycle.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/meteor/server/services/media-call/service.ts">
<violation number="1" location="apps/meteor/server/services/media-call/service.ts:30">
P2: Handle rejections from `sendVoipPushNotification` here; this async emitter callback currently leaves push-send failures as unhandled promise rejections.</violation>
</file>
<file name="apps/meteor/server/services/push/tokenManagement/registerPushToken.ts">
<violation number="1" location="apps/meteor/server/services/push/tokenManagement/registerPushToken.ts:63">
P1: Regular token refreshes should update an existing paired push-token record and clear `voipToken`; this branch inserts a replacement record instead, and duplicate cleanup deletes the original device `_id`, breaking later VoIP updates.
(Based on your team's feedback about regular push refreshes needing to unset stale VoIP tokens instead of replacing the device record.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/meteor/app/push/server/push.ts">
<violation number="1" location="apps/meteor/app/push/server/push.ts:189">
P2: Only increment `countApn` after confirming there is a token to send to; otherwise VoIP notifications with no `voipToken` are reported as delivered even though nothing was sent.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
ee/packages/media-calls/src/server/CallDirector.ts (1)
218-219: Remove the inline rationale comment.Line 218 adds an implementation comment. Please keep the rationale in the PR/ADR and leave the assignment self-explanatory in code.
As per coding guidelines, "Avoid code comments in the implementation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/packages/media-calls/src/server/CallDirector.ts` around lines 218 - 219, Remove the inline rationale comment preceding the _id assignment in CallDirector (the line with "_id: randomUUID()"); simply leave the assignment self-explanatory by deleting the comment text so the code reads only the property assignment. Ensure you only remove the comment and do not alter the _id: randomUUID() expression or surrounding logic.apps/meteor/server/services/media-call/push/getPushNotificationType.ts (1)
10-12: Consider edge case:hangupReason === 'rejected'withoutendedBy.The check at line 10 uses
ORlogic:call.endedBy?.id === call.callee.id || call.hangupReason === 'rejected'. IfhangupReasonis'rejected'butendedByis not the callee (e.g., server-initiated rejection), this would still return'declinedElsewhere'. Verify this matches the intended behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/services/media-call/push/getPushNotificationType.ts` around lines 10 - 12, The current condition in getPushNotificationType uses OR for call.endedBy?.id === call.callee.id || call.hangupReason === 'rejected', which will classify server-initiated 'rejected' hangups as 'declinedElsewhere'; change the logic so 'rejected' only counts when the callee actually ended the call. Update the condition around call.endedBy, call.callee.id and call.hangupReason so that you require endedBy?.id === call.callee.id (or require call.hangupReason === 'rejected' AND endedBy?.id === call.callee.id) instead of the current OR expression.apps/meteor/server/services/push/tokenManagement/registerPushToken.ts (1)
26-41: VoIP token silently dropped when_idis missing.On line 34,
voipTokenis only included in the insert ifdata._idis present:...(data.voipToken && data._id && { voipToken: data.voipToken }). While the API layer validates this (requiringidwhenvoipTokenis provided), ifregisterPushTokenis called from another code path without this validation, thevoipTokenwould be silently ignored.Consider adding a debug log or defensive check here to make the behavior more explicit.
Optional: Add defensive logging
async function insertToken(data: PushTokenData): Promise<IPushToken['_id']> { + if (data.voipToken && !data._id) { + logger.warn({ msg: 'voipToken provided without _id, voipToken will not be stored' }); + } + const insertResult = await PushToken.insertToken({ ...(data._id && { _id: data._id }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/services/push/tokenManagement/registerPushToken.ts` around lines 26 - 41, The insertToken function is currently dropping voipToken when data._id is missing because of the conditional ...(data.voipToken && data._id && { voipToken: data.voipToken }); modify insertToken (and the call to PushToken.insertToken) to explicitly handle the case where data.voipToken is present but data._id is missing: either add a defensive logger warning via logger.warn or throw an Error when data.voipToken && !data._id, so callers are alerted, and then keep the existing insert behavior (include voipToken only when _id exists); reference insertToken, PushToken.insertToken, data.voipToken and data._id when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/app/api/server/v1/push.ts`:
- Around line 149-151: The response schema's voipToken property must allow
null/absent values: update the response schema where voipToken is declared (the
voipToken property in the push token response schema) to include nullable: true
(or mark it optional/null depending on your schema library) so validation
accepts undefined/null values returned from storage; keep the key name voipToken
and ensure any TypeScript type/interfaces that mirror this schema also allow
string | null | undefined.
In `@apps/meteor/server/services/media-call/push/sendVoipPushNotification.ts`:
- Around line 13-24: The projection in getActorUser does not include
freeSwitchExtension but the generic T and later code expect it; update the
options projection object used by Users.findOneById and
Users.findOneByFreeSwitchExtension in getActorUser to include
freeSwitchExtension (e.g. add freeSwitchExtension: 1) so the returned user
satisfies T and user.freeSwitchExtension is available.
In `@apps/meteor/server/services/push/tokenManagement/findDocumentToUpdate.ts`:
- Around line 12-15: The early return in findDocumentToUpdate when
data.voipToken is present forces VoIP payloads into the insert path; change the
guard so it only short-circuits for VoIP-only updates. Concretely, in
findDocumentToUpdate update the condition from "if (data.voipToken) return null"
to only return null when the incoming payload contains voipToken but does NOT
contain any identifying main-token fields (e.g., no data.token, no data.appName,
and no data._id), so that mixed APN+VoIP payloads (token+appName+voipToken) will
fall through and match/update the existing document used by registerPushToken
instead of inserting a new one.
---
Nitpick comments:
In `@apps/meteor/server/services/media-call/push/getPushNotificationType.ts`:
- Around line 10-12: The current condition in getPushNotificationType uses OR
for call.endedBy?.id === call.callee.id || call.hangupReason === 'rejected',
which will classify server-initiated 'rejected' hangups as 'declinedElsewhere';
change the logic so 'rejected' only counts when the callee actually ended the
call. Update the condition around call.endedBy, call.callee.id and
call.hangupReason so that you require endedBy?.id === call.callee.id (or require
call.hangupReason === 'rejected' AND endedBy?.id === call.callee.id) instead of
the current OR expression.
In `@apps/meteor/server/services/push/tokenManagement/registerPushToken.ts`:
- Around line 26-41: The insertToken function is currently dropping voipToken
when data._id is missing because of the conditional ...(data.voipToken &&
data._id && { voipToken: data.voipToken }); modify insertToken (and the call to
PushToken.insertToken) to explicitly handle the case where data.voipToken is
present but data._id is missing: either add a defensive logger warning via
logger.warn or throw an Error when data.voipToken && !data._id, so callers are
alerted, and then keep the existing insert behavior (include voipToken only when
_id exists); reference insertToken, PushToken.insertToken, data.voipToken and
data._id when making the change.
In `@ee/packages/media-calls/src/server/CallDirector.ts`:
- Around line 218-219: Remove the inline rationale comment preceding the _id
assignment in CallDirector (the line with "_id: randomUUID()"); simply leave the
assignment self-explanatory by deleting the comment text so the code reads only
the property assignment. Ensure you only remove the comment and do not alter the
_id: randomUUID() expression or surrounding logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c94fd886-f6f9-491d-9cd5-b6049d59b307
📒 Files selected for processing (20)
apps/meteor/app/api/server/v1/push.tsapps/meteor/app/push/server/apn.tsapps/meteor/app/push/server/definition.tsapps/meteor/app/push/server/fcm.tsapps/meteor/app/push/server/push.tsapps/meteor/server/services/media-call/logger.tsapps/meteor/server/services/media-call/push/getPushNotificationType.tsapps/meteor/server/services/media-call/push/sendVoipPushNotification.tsapps/meteor/server/services/media-call/service.tsapps/meteor/server/services/push/service.tsapps/meteor/server/services/push/tokenManagement/findDocumentToUpdate.tsapps/meteor/server/services/push/tokenManagement/registerPushToken.tsee/packages/media-calls/src/definition/IMediaCallServer.tsee/packages/media-calls/src/internal/agents/UserActorAgent.tsee/packages/media-calls/src/server/CallDirector.tsee/packages/media-calls/src/server/MediaCallServer.tspackages/core-typings/src/IPushNotificationConfig.tspackages/core-typings/src/IPushToken.tspackages/model-typings/src/models/IPushTokenModel.tspackages/models/src/models/PushToken.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). (1)
- GitHub Check: cubic · AI code reviewer
🧰 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/push/getPushNotificationType.tspackages/core-typings/src/IPushNotificationConfig.tsee/packages/media-calls/src/definition/IMediaCallServer.tsapps/meteor/server/services/push/tokenManagement/findDocumentToUpdate.tsapps/meteor/server/services/media-call/push/sendVoipPushNotification.tspackages/core-typings/src/IPushToken.tsapps/meteor/server/services/media-call/logger.tsee/packages/media-calls/src/server/CallDirector.tsee/packages/media-calls/src/internal/agents/UserActorAgent.tsapps/meteor/app/push/server/apn.tsapps/meteor/server/services/push/service.tsapps/meteor/app/push/server/fcm.tsapps/meteor/app/push/server/definition.tsapps/meteor/server/services/push/tokenManagement/registerPushToken.tsapps/meteor/server/services/media-call/service.tspackages/model-typings/src/models/IPushTokenModel.tspackages/models/src/models/PushToken.tsee/packages/media-calls/src/server/MediaCallServer.tsapps/meteor/app/api/server/v1/push.tsapps/meteor/app/push/server/push.ts
🧠 Learnings (16)
📓 Common learnings
Learnt from: pierre-lehnen-rc
Repo: RocketChat/Rocket.Chat PR: 39174
File: apps/meteor/server/services/push/tokenManagement/registerPushToken.ts:47-49
Timestamp: 2026-03-02T16:21:44.002Z
Learning: In the Rocket.Chat push token system (apps/meteor/server/services/push/tokenManagement/registerPushToken.ts), the main (non-VoIP) token is responsible for keeping authToken, appName, and userId updated. VoIP tokens are supplementary—when a device already has a non-VoIP token registered, VoIP token updates only modify the voipToken field, delegating metadata management to the main token registration flow.
📚 Learning: 2026-03-02T16:21:44.002Z
Learnt from: pierre-lehnen-rc
Repo: RocketChat/Rocket.Chat PR: 39174
File: apps/meteor/server/services/push/tokenManagement/registerPushToken.ts:47-49
Timestamp: 2026-03-02T16:21:44.002Z
Learning: In the Rocket.Chat push token system (apps/meteor/server/services/push/tokenManagement/registerPushToken.ts), the main (non-VoIP) token is responsible for keeping authToken, appName, and userId updated. VoIP tokens are supplementary—when a device already has a non-VoIP token registered, VoIP token updates only modify the voipToken field, delegating metadata management to the main token registration flow.
Applied to files:
apps/meteor/server/services/media-call/push/getPushNotificationType.tspackages/core-typings/src/IPushNotificationConfig.tsapps/meteor/server/services/push/tokenManagement/findDocumentToUpdate.tsapps/meteor/server/services/media-call/push/sendVoipPushNotification.tspackages/core-typings/src/IPushToken.tsapps/meteor/app/push/server/apn.tsapps/meteor/server/services/push/service.tsapps/meteor/app/push/server/fcm.tsapps/meteor/app/push/server/definition.tsapps/meteor/server/services/push/tokenManagement/registerPushToken.tspackages/model-typings/src/models/IPushTokenModel.tspackages/models/src/models/PushToken.tsapps/meteor/app/api/server/v1/push.tsapps/meteor/app/push/server/push.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/push/getPushNotificationType.tspackages/core-typings/src/IPushNotificationConfig.tsee/packages/media-calls/src/definition/IMediaCallServer.tsapps/meteor/server/services/push/tokenManagement/findDocumentToUpdate.tsapps/meteor/server/services/media-call/push/sendVoipPushNotification.tspackages/core-typings/src/IPushToken.tsapps/meteor/server/services/media-call/logger.tsee/packages/media-calls/src/server/CallDirector.tsee/packages/media-calls/src/internal/agents/UserActorAgent.tsapps/meteor/app/push/server/apn.tsapps/meteor/server/services/push/service.tsapps/meteor/app/push/server/fcm.tsapps/meteor/app/push/server/definition.tsapps/meteor/server/services/push/tokenManagement/registerPushToken.tsapps/meteor/server/services/media-call/service.tspackages/model-typings/src/models/IPushTokenModel.tspackages/models/src/models/PushToken.tsee/packages/media-calls/src/server/MediaCallServer.tsapps/meteor/app/api/server/v1/push.tsapps/meteor/app/push/server/push.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/push/getPushNotificationType.tspackages/core-typings/src/IPushNotificationConfig.tsee/packages/media-calls/src/definition/IMediaCallServer.tsapps/meteor/server/services/push/tokenManagement/findDocumentToUpdate.tsapps/meteor/server/services/media-call/push/sendVoipPushNotification.tspackages/core-typings/src/IPushToken.tsapps/meteor/server/services/media-call/logger.tsee/packages/media-calls/src/server/CallDirector.tsee/packages/media-calls/src/internal/agents/UserActorAgent.tsapps/meteor/app/push/server/apn.tsapps/meteor/server/services/push/service.tsapps/meteor/app/push/server/fcm.tsapps/meteor/app/push/server/definition.tsapps/meteor/server/services/push/tokenManagement/registerPushToken.tsapps/meteor/server/services/media-call/service.tspackages/model-typings/src/models/IPushTokenModel.tspackages/models/src/models/PushToken.tsee/packages/media-calls/src/server/MediaCallServer.tsapps/meteor/app/api/server/v1/push.tsapps/meteor/app/push/server/push.ts
📚 Learning: 2025-11-17T14:30:36.271Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 37491
File: packages/desktop-api/src/index.ts:17-27
Timestamp: 2025-11-17T14:30:36.271Z
Learning: In the Rocket.Chat desktop API (`packages/desktop-api/src/index.ts`), the `CustomNotificationOptions` type has an optional `id` field by design. Custom notifications dispatched without an ID cannot be closed programmatically using `closeCustomNotification`, and this is intentional.
Applied to files:
packages/core-typings/src/IPushNotificationConfig.tsapps/meteor/app/push/server/push.ts
📚 Learning: 2025-11-19T18:20:37.116Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: apps/meteor/server/services/media-call/service.ts:141-141
Timestamp: 2025-11-19T18:20:37.116Z
Learning: In apps/meteor/server/services/media-call/service.ts, the sendHistoryMessage method should use call.caller.id or call.createdBy?.id as the message author, not call.transferredBy?.id. Even for transferred calls, the message should appear in the DM between the two users who are calling each other, not sent by the person who transferred the call.
Applied to files:
apps/meteor/server/services/media-call/push/sendVoipPushNotification.tsapps/meteor/server/services/media-call/logger.tsee/packages/media-calls/src/server/CallDirector.tsee/packages/media-calls/src/internal/agents/UserActorAgent.tsapps/meteor/server/services/media-call/service.ts
📚 Learning: 2026-02-20T09:04:55.725Z
Learnt from: Shreyas2004wagh
Repo: RocketChat/Rocket.Chat PR: 38681
File: apps/meteor/server/modules/streamer/streamer.module.ts:307-313
Timestamp: 2026-02-20T09:04:55.725Z
Learning: In apps/meteor/server/modules/streamer/streamer.module.ts, the catch block in sendToManySubscriptions intentionally uses SystemLogger.debug (not error or warn) for per-subscription delivery failures to keep logs less noisy, as this was a deliberate design choice reviewed and approved by KevLehman.
Applied to files:
apps/meteor/server/services/media-call/logger.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:
ee/packages/media-calls/src/server/CallDirector.tsapps/meteor/server/services/media-call/service.tsee/packages/media-calls/src/server/MediaCallServer.ts
📚 Learning: 2026-02-26T19:22:36.646Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/views/CallHistoryContextualbar/CallHistoryActions.tsx:40-40
Timestamp: 2026-02-26T19:22:36.646Z
Learning: In packages/ui-voip/src/views/CallHistoryContextualbar/CallHistoryActions.tsx, when the media session state is 'unavailable', the voiceCall action is not included in the actions object passed to CallHistoryActions, so it won't appear in the menu at all. The action filtering happens upstream before getItems is called, preventing any tooltip confusion for the unavailable state.
Applied to files:
ee/packages/media-calls/src/internal/agents/UserActorAgent.tsapps/meteor/server/services/media-call/service.tsee/packages/media-calls/src/server/MediaCallServer.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:
apps/meteor/app/push/server/apn.tsapps/meteor/server/services/push/service.ts
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/server/services/push/service.tsapps/meteor/app/push/server/fcm.tsapps/meteor/app/push/server/definition.ts
📚 Learning: 2025-09-30T13:00:05.465Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 36990
File: apps/meteor/ee/server/apps/storage/AppRealStorage.ts:55-58
Timestamp: 2025-09-30T13:00:05.465Z
Learning: In AppRealStorage (apps/meteor/ee/server/apps/storage/AppRealStorage.ts), the `remove` method is designed to be idempotent and returns `{ success: true }` unconditionally because the goal is to ensure the app is removed, not to distinguish whether this specific call performed the deletion. Database errors will throw exceptions.
Applied to files:
apps/meteor/server/services/push/service.tspackages/models/src/models/PushToken.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:
packages/model-typings/src/models/IPushTokenModel.tspackages/models/src/models/PushToken.tsapps/meteor/app/api/server/v1/push.ts
📚 Learning: 2026-03-09T18:39:14.020Z
Learnt from: Harxhit
Repo: RocketChat/Rocket.Chat PR: 39476
File: apps/meteor/server/methods/addAllUserToRoom.ts:0-0
Timestamp: 2026-03-09T18:39:14.020Z
Learning: In apps/meteor/server/methods/addAllUserToRoom.ts, the implementation uses a single cursor pass (Users.find(userFilter).batchSize(100)) that collects both the full user objects (collectedUsers: IUser[]) and their usernames (usernames: string[]) in one iteration. `beforeAddUserToRoom` is then called once with the full usernames batch (preserving batch-validation semantics), and the subsequent subscription/message processing loop iterates over the same stable `collectedUsers` array — no second DB query is made. This avoids any race condition between validation and processing while preserving the original batch-validation behavior.
Applied to files:
packages/models/src/models/PushToken.ts
📚 Learning: 2026-02-23T17:53:06.802Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:06.802Z
Learning: During PR reviews that touch endpoint files under apps/meteor/app/api/server/v1, enforce strict scope: if a PR targets a specific endpoint (e.g., rooms.favorite), do not propose changes to unrelated endpoints (e.g., rooms.invite) unless maintainers explicitly request them. Focus feedback on the touched endpoint's behavior, API surface, and related tests; avoid broad cross-endpoint changes in the same PR unless requested.
Applied to files:
apps/meteor/app/api/server/v1/push.ts
📚 Learning: 2026-02-24T19:09:01.522Z
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:01.522Z
Learning: In Rocket.Chat OpenAPI migration PRs for endpoints under apps/meteor/app/api/server/v1, avoid introducing logic changes. Only perform scope-tight changes that preserve behavior; style-only cleanups (e.g., removing inline comments) may be deferred to follow-ups to keep the migration PR focused.
Applied to files:
apps/meteor/app/api/server/v1/push.ts
🔇 Additional comments (27)
apps/meteor/app/push/server/fcm.ts (3)
11-15: LGTM! Optional notification fields support VoIP use cases.Making
titleandbodyoptional inFCMNotificationFieldcorrectly aligns with the broader push notification changes where VoIP notifications may not include visible notification content.
142-154: LGTM! Conditional notification field construction is correct.The conditional spread pattern at lines 143-144 ensures
titleandbodyare only included when present, and line 149 correctly omits thenotificationfield entirely when empty. This allows data-only FCM messages for VoIP.
186-189: LGTM! Token removal simplified to use string directly.The change to pass
tokendirectly to_removeTokenaligns with the updatedPushToken.removeAllByTokenStringAPI that accepts a raw token string instead of an object structure.apps/meteor/server/services/media-call/service.ts (2)
16-17: LGTM! Clean import structure for VoIP push integration.The imports correctly bring in the module-scoped logger and the new
sendVoipPushNotificationhandler.
30-30: LGTM! Event wiring for VoIP push notifications follows established patterns.The
pushNotificationRequestevent listener correctly delegates tosendVoipPushNotification, following the same pattern as the existingsignalRequest,callUpdated, andhistoryUpdatehandlers.apps/meteor/server/services/push/service.ts (1)
39-44: LGTM! Duplicate token removal parameters align with updated token management API.The
removeDuplicateTokenscall correctly passes the required fields (_id,token,appName,authToken) for identifying and removing duplicate tokens while preserving the newly registered token.ee/packages/media-calls/src/internal/agents/UserActorAgent.ts (3)
54-64: LGTM! End notification correctly gated for callee role.The
'end'push notification is appropriately sent only for the callee role before sending the hangup signal, allowing the callee's device to handle call termination.
74-85: LGTM! New call notification correctly follows signal dispatch.The
'new'push notification is sent aftersendSignal(buildNewCallSignal(...)), which is the correct order—the signal goes first to online clients, then the push notification serves as a backup to wake offline devices.
24-52: Push notification timing is intentional and correctly designed.The
onCallAcceptedmethod is explicitly documented inIMediaCallAgent.tsas being "called when the call was accepted, even if the webrtc negotiation is pending." The early return when the offer is unavailable (lines 38-42) is by design—it allows the 'answer' push event to signal call acceptance independently of WebRTC negotiation readiness, which is handled separately viaonRemoteDescriptionChangedwhen the SDP becomes available. No changes needed.> Likely an incorrect or invalid review comment.apps/meteor/app/push/server/definition.ts (1)
20-46: LGTM! Type changes enable VoIP-specific push notification flows.Making
from,title, andtextoptional correctly supports VoIP push notifications that may be data-only. The newuseVoipTokenfield enables routing to the dedicated VoIP push token. The context snippets confirm thatpush.ts,apn.ts, andfcm.tsall handle these optional fields with appropriate conditional checks.ee/packages/media-calls/src/server/MediaCallServer.ts (2)
9-14: LGTM! Import correctly expanded for VoIP push notification types.The import now includes
VoipPushNotificationEventTypewhich is used in the newsendPushNotificationmethod signature.
78-82: LGTM! Push notification method follows established patterns.The
sendPushNotificationmethod correctly follows the same pattern assendSignal,reportCallUpdate, andupdateCallHistory—logging debug information and emitting an event for the service layer to handle.apps/meteor/app/push/server/apn.ts (3)
37-42: LGTM! VoIP-specific APN configuration is correct.The 60-second expiry for VoIP notifications is appropriate for time-sensitive call alerts, and
pushType: 'voip'is the correct setting for iOS VoIP push notifications via PushKit.
59-79: LGTM! Conditional field assignments handle optional values correctly.Each field (
category,body,title,from) is now guarded with an existence check before assignment, preventing undefined values from being set on the APN notification object.
27-27: LGTM! Token removal callback simplified to accept string directly.The
_removeTokensignature change to accept a plain string aligns with the updatedPushToken.removeAllByTokenStringAPI used by the caller inpush.ts.Also applies to: 98-98
apps/meteor/server/services/media-call/push/getPushNotificationType.ts (1)
5-23: No actionable changes needed. The function correctly handles the'answeredElsewhere'notification type through an early return guard in the consuming function (sendVoipPushNotification.ts, line 69): whencall.acceptedAtis set, notifications are only sent for the'answer'event, not for subsequent'end'events. The'answeredElsewhere'type is therefore only returned when appropriate—to notify other devices that a call was answered—and the concern about notifying the device that completed the call is prevented by the guard condition upstream.apps/meteor/server/services/media-call/push/sendVoipPushNotification.ts (2)
85-103: LGTM!The notification payload structure is well-designed with appropriate fields for VoIP call notifications. The conditional
skipTokenIdusingcontractIdcorrectly prevents sending state-change notifications to the device that already handled the call.
56-80: LGTM!The early return logic correctly handles edge cases: skipping notifications for already-accepted calls (except 'answer' events), and ensuring state consistency between event types and notification types.
apps/meteor/server/services/push/tokenManagement/registerPushToken.ts (2)
12-24: LGTM!The
canModifyTokenDocumentlogic correctly ensures VoIP tokens can only be modified via explicit_idmatch, preventing accidental overwrites. This aligns with the design where VoIP tokens are supplementary to the main token. Based on learnings: "VoIP tokens are supplementary—when a device already has a non-VoIP token registered, VoIP token updates only modify the voipToken field, delegating metadata management to the main token registration flow."
60-68: LGTM!The refactored
registerPushTokenfunction cleanly orchestrates the token registration flow with proper separation of concerns between finding, inserting, and updating tokens.apps/meteor/app/api/server/v1/push.ts (2)
166-170: LGTM!Good defensive validation ensuring
voipTokencannot be registered without a deviceid. This prevents the silent token drop issue in the downstream registration logic.
24-30: LGTM!Good refactoring to use shared
IPushTokenTypesandpushTokenTypesfrom core-typings, ensuring consistency across the codebase. ThecleanTokenResultfunction properly sanitizes the response.Also applies to: 80-97
apps/meteor/app/push/server/push.ts (5)
331-346: LGTM!The VoIP-specific retry limit (
maxTries: 1for VoIP vsPUSH_GATEWAY_MAX_ATTEMPTSfor regular notifications) is a sensible design choice. VoIP notifications are time-sensitive, so avoiding extended retries prevents stale call notifications. The token existence check on line 343 properly handles cases where a VoIP token may not be registered.
356-384: LGTM!The
skipTokenIdoption is well-implemented, allowing VoIP notifications to exclude the device that already handled the call action. The conditional betweenfindTokensByUserIdExceptIdandfindAllTokensByUserIdcleanly handles both cases.
429-454: LGTM!The validation schema changes correctly accommodate VoIP notifications which may not have
from,title, ortextfields. The addition ofuseVoipTokenvalidation ensures type safety.
188-196: LGTM!The APN notification path correctly handles VoIP tokens: selecting the appropriate token based on
useVoipToken, setting the topic suffix to.voipfor VoIP notifications, and properly guarding against missing tokens before sending.
170-174: LGTM!The
removeTokenmethod signature change to accept a token string directly simplifies the interface and aligns with how it's called from APN/FCM error handlers. The error handling with logging prevents unhandled promise rejections.
apps/meteor/server/services/push/tokenManagement/findDocumentToUpdate.ts
Show resolved
Hide resolved
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
There was a problem hiding this comment.
1 issue found across 29 files
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/models/src/models/PushToken.ts">
<violation number="1" location="packages/models/src/models/PushToken.ts:146">
P2: New `voipToken` queries were added without a corresponding index, which can cause collection scans and degrade push-token cleanup performance.</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 26 files
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="ee/packages/media-calls/src/internal/SignalProcessor.ts">
<violation number="1" location="ee/packages/media-calls/src/internal/SignalProcessor.ts:163">
P2: Only emit the "trying" notification when the call is still pending; otherwise reconnecting to an active/accepted call will spam the other side with a misleading "trying" notification.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
dionisio-bot
bot
dismissed
KevLehman’s stale review
The base branch was changed.
4 participants
Proposed changes (including videos or screenshots)
Issue(s)
VMUX-47
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Improvements