chore: accept additional push tokens intended for voip to iOS devices#39174
chore: accept additional push tokens intended for voip to iOS devices#39174dionisio-bot[bot] merged 7 commits intodevelopfrom
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
WalkthroughAdds optional Apple VoIP push token support by accepting, validating, persisting, and returning Changes
Sequence DiagramsequenceDiagram
participant Client
participant API as API Endpoint (push.ts)
participant Service as Service Layer (registerPushToken)
participant Finder as Document Finder (findDocumentToUpdate)
participant Model as Model Layer (PushToken)
participant DB as Database
Client->>API: POST /push { type, token, voipToken?, id? }
API->>API: Validate payload (type ∈ IPushTokenTypes)
API->>Service: registerPushToken(data)
Service->>Finder: findDocumentToUpdate(data)
alt voipToken present
Finder-->>Service: null (skip token+appName lookup)
else
Finder->>DB: findOne by token & appName
DB-->>Finder: existing document or null
Finder-->>Service: document | null
end
Service->>Service: canModifyTokenDocument(existing, data)
alt insert
Service->>Model: insertToken(data)
Model->>DB: insert doc (include voipToken if provided)
DB-->>Model: inserted _id
Model-->>Service: _id
else update
Service->>Model: refreshTokenById(id, updateData)
Model->>DB: update (set/unset voipToken)
DB-->>Model: update result
Model-->>Service: update result
end
Service->>Model: removeDuplicateTokens({ _id, token, appName, authToken })
Model->>DB: delete duplicates (match token+appName OR authToken, _id != given)
DB-->>Model: delete result
Service-->>API: return token _id / result
API-->>Client: 200 OK with token data (includes voipToken if present)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested labelstype: feature 🚥 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 #39174 +/- ##
===========================================
- Coverage 70.95% 70.88% -0.08%
===========================================
Files 3196 3196
Lines 113282 113293 +11
Branches 20489 20607 +118
===========================================
- Hits 80384 80308 -76
- Misses 30853 30938 +85
- Partials 2045 2047 +2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/meteor/server/services/push/tokenManagement/findDocumentToUpdate.ts (1)
12-13: Remove inline implementation comment.Keep the guard self-explanatory via naming/structure and drop the comment line.
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 `@apps/meteor/server/services/push/tokenManagement/findDocumentToUpdate.ts` around lines 12 - 13, Remove the inline comment "// VoIP tokens MUST match the id" and make the guard self-explanatory by extracting the condition into a named boolean (e.g., const isVoipToken = data.token?.['apn.voip'] || data.voipToken) and then use if (isVoipToken) in findDocumentToUpdate; this eliminates the implementation comment while keeping intent clear via the name.apps/meteor/server/services/push/tokenManagement/registerPushToken.ts (1)
13-20: Remove inline implementation comments.Please replace these comments with clear naming/flow and keep implementation code comment-free.
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 `@apps/meteor/server/services/push/tokenManagement/registerPushToken.ts` around lines 13 - 20, Remove the inline comments and make the flow self-documenting by introducing descriptive boolean variables and early returns; e.g., compute hasNoVoipOnEitherSide = !doc.voipToken && !data.voipToken and return true if that is true, and compute voipIdMissingOrMismatch = !data._id || data._id !== doc._id and return false if that is true; keep the logic around doc.voipToken, data.voipToken, data._id and doc._id but eliminate the comments so the intent is expressed via clear variable names and control flow instead of inline comments.
🤖 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 24-29: The PushTokenPOST type currently allows id to be optional
which permits apn/gcm registrations without a device identifier; change the
declaration in PushTokenPOST from id?: string to id: string so id is required,
then update all code paths that construct or validate PushTokenPOST (e.g., any
request handlers/endpoints that accept push tokens and any runtime validators)
to enforce and reject requests missing id, and adjust related tests/schemas and
any uses of IPushTokenTypes to conform to the new required id field so tokens
are always pairable by device identifier.
---
Nitpick comments:
In `@apps/meteor/server/services/push/tokenManagement/findDocumentToUpdate.ts`:
- Around line 12-13: Remove the inline comment "// VoIP tokens MUST match the
id" and make the guard self-explanatory by extracting the condition into a named
boolean (e.g., const isVoipToken = data.token?.['apn.voip'] || data.voipToken)
and then use if (isVoipToken) in findDocumentToUpdate; this eliminates the
implementation comment while keeping intent clear via the name.
In `@apps/meteor/server/services/push/tokenManagement/registerPushToken.ts`:
- Around line 13-20: Remove the inline comments and make the flow
self-documenting by introducing descriptive boolean variables and early returns;
e.g., compute hasNoVoipOnEitherSide = !doc.voipToken && !data.voipToken and
return true if that is true, and compute voipIdMissingOrMismatch = !data._id ||
data._id !== doc._id and return false if that is true; keep the logic around
doc.voipToken, data.voipToken, data._id and doc._id but eliminate the comments
so the intent is expressed via clear variable names and control flow instead of
inline comments.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/meteor/app/api/server/v1/push.tsapps/meteor/server/services/push/service.tsapps/meteor/server/services/push/tokenManagement/findDocumentToUpdate.tsapps/meteor/server/services/push/tokenManagement/registerPushToken.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). (20)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (4/5)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (4/5)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (4/5)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (4/5)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (4/5)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (4/5)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (4/5)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (4/5)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (4/5)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (4/5)
🧰 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/push/tokenManagement/findDocumentToUpdate.tsapps/meteor/server/services/push/tokenManagement/registerPushToken.tsapps/meteor/server/services/push/service.tspackages/model-typings/src/models/IPushTokenModel.tspackages/models/src/models/PushToken.tspackages/core-typings/src/IPushToken.tsapps/meteor/app/api/server/v1/push.ts
🧠 Learnings (9)
📓 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: 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/push/tokenManagement/findDocumentToUpdate.tsapps/meteor/server/services/push/tokenManagement/registerPushToken.tsapps/meteor/server/services/push/service.tspackages/model-typings/src/models/IPushTokenModel.tspackages/models/src/models/PushToken.tspackages/core-typings/src/IPushToken.tsapps/meteor/app/api/server/v1/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/push/tokenManagement/findDocumentToUpdate.tsapps/meteor/server/services/push/tokenManagement/registerPushToken.tsapps/meteor/server/services/push/service.tspackages/model-typings/src/models/IPushTokenModel.tspackages/models/src/models/PushToken.tspackages/core-typings/src/IPushToken.tsapps/meteor/app/api/server/v1/push.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/tokenManagement/registerPushToken.tsapps/meteor/server/services/push/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:
apps/meteor/server/services/push/service.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:
apps/meteor/app/api/server/v1/push.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
🧬 Code graph analysis (5)
apps/meteor/server/services/push/tokenManagement/registerPushToken.ts (5)
packages/core-typings/src/utils.ts (1)
Optional(3-3)packages/core-typings/src/IPushToken.ts (1)
IPushToken(8-17)packages/models/src/models/PushToken.ts (1)
insertToken(49-55)apps/meteor/server/services/push/logger.ts (1)
logger(3-3)apps/meteor/server/services/push/tokenManagement/findDocumentToUpdate.ts (1)
findDocumentToUpdate(4-22)
apps/meteor/server/services/push/service.ts (1)
packages/models/src/index.ts (1)
PushToken(175-175)
packages/model-typings/src/models/IPushTokenModel.ts (2)
packages/core-typings/src/IPushToken.ts (1)
IPushToken(8-17)apps/meteor/app/push/server/methods.ts (1)
id(66-76)
packages/models/src/models/PushToken.ts (1)
packages/core-typings/src/IPushToken.ts (1)
IPushToken(8-17)
apps/meteor/app/api/server/v1/push.ts (1)
packages/core-typings/src/IPushToken.ts (3)
IPushTokenTypes(6-6)pushTokenTypes(4-4)IPushToken(8-17)
🔇 Additional comments (5)
packages/core-typings/src/IPushToken.ts (1)
4-6: Good centralization of push-token typing for VoIP support.The
pushTokenTypessource-of-truth plusIPushTokenTypesand optionalvoipTokenkeep downstream contracts consistent.Also applies to: 16-16
packages/model-typings/src/models/IPushTokenModel.ts (1)
16-21: Model-typing updates are aligned with the new token lifecycle.The added
voipTokenpath and dedup API shape are consistent with the rest of the PR surface.apps/meteor/server/services/push/service.ts (1)
39-44: Dedup call-site is correctly migrated to the new API shape.Passing
_id,token,appName, andauthTokenas one payload keeps this layer aligned with the model contract.packages/models/src/models/PushToken.ts (1)
57-77: Refresh + dedicated VoIP update methods are clearly separated.The API split between
refreshTokenByIdandsetVoipTokenByIdkeeps update intent explicit.apps/meteor/app/api/server/v1/push.ts (1)
2-3: Token-type centralization is a solid improvement.Using shared typings/constants avoids enum drift between API validation and model typing.
Also applies to: 26-27, 40-41
apps/meteor/server/services/push/tokenManagement/registerPushToken.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
1 issue found across 7 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/api/server/v1/push.ts">
<violation number="1" location="apps/meteor/app/api/server/v1/push.ts:167">
P2: Use a structured API error (e.g., Meteor.Error or API.v1.badRequest) for invalid parameters so the endpoint returns the expected 400 response instead of a generic 500.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
61e99d5 to
8f27671
Compare
There was a problem hiding this comment.
2 issues found across 7 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/server/services/push/tokenManagement/registerPushToken.ts">
<violation number="1" location="apps/meteor/server/services/push/tokenManagement/registerPushToken.ts:63">
P1: Inserting on `!canModifyTokenDocument` can cause existing VoIP token records to be replaced and then removed by duplicate cleanup, losing `voipToken` for the device.</violation>
</file>
<file name="packages/models/src/models/PushToken.ts">
<violation number="1" location="packages/models/src/models/PushToken.ts:71">
P2: This unsets `voipToken` whenever the field is omitted, which can unintentionally delete an existing VoIP token during normal token refreshes.</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.
♻️ Duplicate comments (1)
apps/meteor/app/api/server/v1/push.ts (1)
24-25:⚠️ Potential issue | 🟠 MajorRequire
idfor every push token registration, not only VoIP payloads.Line 25 keeps
idoptional, and Lines 168-170 only enforce it whenvoipTokenexists. This still allows regular token registrations without device identity, which breaks deterministic regular↔VoIP pairing by device.Suggested fix
type PushTokenPOST = { - id?: string; + id: string; type: IPushTokenTypes; value: string; appName: string; voipToken?: string; }; @@ id: { type: 'string', - nullable: true, }, @@ - required: ['type', 'value', 'appName'], + required: ['id', 'type', 'value', 'appName'], @@ - if (voipToken && !id) { - return API.v1.failure('voip-tokens-must-specify-device-id'); + if (!id) { + return API.v1.failure('push-tokens-must-specify-device-id'); }Also applies to: 35-38, 56-57, 166-170
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/v1/push.ts` around lines 24 - 25, The id field must be required for all push token registrations: update the PushTokenPOST type (and any related types at the other occurrences) to make id non-optional, and change the runtime validation in the push token registration handler (the code that currently only enforces id when voipToken exists—look for the block checking voipToken around the existing lines 166-170) to require req.body.id for every registration path instead of only for VoIP; remove the conditional check and throw/return an error when id is missing for any token registration.
🧹 Nitpick comments (2)
apps/meteor/server/services/push/tokenManagement/findDocumentToUpdate.ts (1)
12-12: Drop the inline implementation comment.Please remove the comment on Line 12 and keep intent encoded in naming/flow only.
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 `@apps/meteor/server/services/push/tokenManagement/findDocumentToUpdate.ts` at line 12, Remove the inline implementation comment "// VoIP tokens MUST match the id" from findDocumentToUpdate.ts; instead ensure the intent is captured in code by renaming variables or functions (e.g., use a clear name like voipTokenMustMatchId or enforceVoipIdMatch in the matching logic inside findDocumentToUpdate or its helper) and keep the check/flow explicit so no inline comment is needed.apps/meteor/server/services/push/tokenManagement/registerPushToken.ts (1)
13-20: Remove inline comments in implementation code.The comments on Lines 13 and 18 should be removed per repository style guidance.
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 `@apps/meteor/server/services/push/tokenManagement/registerPushToken.ts` around lines 13 - 20, Remove the two inline implementation comments in registerPushToken.ts that precede the voip checks: the comment before the if (!doc.voipToken && !data.voipToken) return true; and the comment before the id check if (!data._id || data._id !== doc._id) return false;. Leave the conditional logic (the checks against doc.voipToken, data.voipToken and the data._id vs doc._id comparison) unchanged and ensure no other comments are removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/meteor/app/api/server/v1/push.ts`:
- Around line 24-25: The id field must be required for all push token
registrations: update the PushTokenPOST type (and any related types at the other
occurrences) to make id non-optional, and change the runtime validation in the
push token registration handler (the code that currently only enforces id when
voipToken exists—look for the block checking voipToken around the existing lines
166-170) to require req.body.id for every registration path instead of only for
VoIP; remove the conditional check and throw/return an error when id is missing
for any token registration.
---
Nitpick comments:
In `@apps/meteor/server/services/push/tokenManagement/findDocumentToUpdate.ts`:
- Line 12: Remove the inline implementation comment "// VoIP tokens MUST match
the id" from findDocumentToUpdate.ts; instead ensure the intent is captured in
code by renaming variables or functions (e.g., use a clear name like
voipTokenMustMatchId or enforceVoipIdMatch in the matching logic inside
findDocumentToUpdate or its helper) and keep the check/flow explicit so no
inline comment is needed.
In `@apps/meteor/server/services/push/tokenManagement/registerPushToken.ts`:
- Around line 13-20: Remove the two inline implementation comments in
registerPushToken.ts that precede the voip checks: the comment before the if
(!doc.voipToken && !data.voipToken) return true; and the comment before the id
check if (!data._id || data._id !== doc._id) return false;. Leave the
conditional logic (the checks against doc.voipToken, data.voipToken and the
data._id vs doc._id comparison) unchanged and ensure no other comments are
removed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c4d190f4-b3ad-4861-94e8-6ad33b961f6e
📒 Files selected for processing (6)
apps/meteor/app/api/server/v1/push.tsapps/meteor/server/services/push/tokenManagement/findDocumentToUpdate.tsapps/meteor/server/services/push/tokenManagement/registerPushToken.tspackages/core-typings/src/IPushToken.tspackages/model-typings/src/models/IPushTokenModel.tspackages/models/src/models/PushToken.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/models/src/models/PushToken.ts
- packages/core-typings/src/IPushToken.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/push/tokenManagement/registerPushToken.tsapps/meteor/app/api/server/v1/push.tsapps/meteor/server/services/push/tokenManagement/findDocumentToUpdate.tspackages/model-typings/src/models/IPushTokenModel.ts
🧠 Learnings (15)
📓 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.
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.
📚 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/push/tokenManagement/registerPushToken.tsapps/meteor/app/api/server/v1/push.tsapps/meteor/server/services/push/tokenManagement/findDocumentToUpdate.tspackages/model-typings/src/models/IPushTokenModel.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/tokenManagement/registerPushToken.tsapps/meteor/app/api/server/v1/push.tsapps/meteor/server/services/push/tokenManagement/findDocumentToUpdate.ts
📚 Learning: 2025-11-19T12:32:29.696Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37547
File: packages/i18n/src/locales/en.i18n.json:634-634
Timestamp: 2025-11-19T12:32:29.696Z
Learning: Repo: RocketChat/Rocket.Chat
Context: i18n workflow
Learning: In this repository, new translation keys should be added to packages/i18n/src/locales/en.i18n.json only; other locale files are populated via the external translation pipeline and/or fall back to English. Do not request adding the same key to all locale files in future reviews.
Applied to files:
apps/meteor/server/services/push/tokenManagement/registerPushToken.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/server/services/push/tokenManagement/registerPushToken.ts
📚 Learning: 2026-02-23T17:53:18.785Z
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:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
Applied to files:
apps/meteor/server/services/push/tokenManagement/registerPushToken.ts
📚 Learning: 2025-11-10T19:06:20.146Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37408
File: apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx:53-69
Timestamp: 2025-11-10T19:06:20.146Z
Learning: In the Rocket.Chat repository, do not provide suggestions or recommendations about code sections marked with TODO comments. The maintainers have already identified these as future work and external reviewers lack the full context about implementation plans and timing.
Applied to files:
apps/meteor/server/services/push/tokenManagement/registerPushToken.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/push/tokenManagement/registerPushToken.tsapps/meteor/app/api/server/v1/push.tsapps/meteor/server/services/push/tokenManagement/findDocumentToUpdate.tspackages/model-typings/src/models/IPushTokenModel.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/push/tokenManagement/registerPushToken.tsapps/meteor/app/api/server/v1/push.tsapps/meteor/server/services/push/tokenManagement/findDocumentToUpdate.tspackages/model-typings/src/models/IPushTokenModel.ts
📚 Learning: 2025-10-06T20:32:23.658Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/utilities.ts:557-573
Timestamp: 2025-10-06T20:32:23.658Z
Learning: In packages/apps-engine/tests/test-data/utilities.ts, the field name `isSubscripbedViaBundle` in the `IMarketplaceSubscriptionInfo` type should not be flagged as a typo, as it may match the upstream API's field name.
Applied to files:
apps/meteor/app/api/server/v1/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:
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
📚 Learning: 2026-01-26T18:26:01.279Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38227
File: apps/meteor/app/api/server/router.ts:44-49
Timestamp: 2026-01-26T18:26:01.279Z
Learning: In apps/meteor/app/api/server/router.ts, when retrieving bodyParams and queryParams from the Hono context via c.get(), do not add defensive defaults (e.g., ?? {}). The code should fail fast if these parameters are missing, as endpoint handlers expect them to be present and breaking here helps surface parsing problems rather than hiding them.
Applied to files:
apps/meteor/app/api/server/v1/push.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/app/api/server/v1/push.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
🔇 Additional comments (3)
packages/model-typings/src/models/IPushTokenModel.ts (1)
14-20: Interface changes look consistent with the new token lifecycle.
refreshTokenByIdnow acceptsvoipToken, and the dedup method shift toremoveDuplicateTokens(...)matches the new token-data flow.apps/meteor/server/services/push/tokenManagement/registerPushToken.ts (1)
43-50: VoIP update behavior matches the intended ownership model.Updating
voipTokenconditionally while still routing main-token metadata through the regular refresh path is consistent with the expected flow.Based on learnings "the main (non-VoIP) token is responsible for keeping authToken, appName, and userId updated… VoIP token updates only modify the voipToken field".
apps/meteor/app/api/server/v1/push.ts (1)
26-27: No issue found: VoIP tokens are properly separated from regular token routing.The
pushTokenTypesenum (line 41) restricts thetypeparameter to only'gcm'or'apn'—'apn.voip'is not an option. VoIP tokens are handled separately via thevoipTokenparameter (line 29), passed independently toregisterPushToken()at line 184. The response schema correctly models onlyapnandgcmkeys in thetokenobject.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/app/api/server/v1/push.ts (1)
149-151: Consider addingnullable: truefor consistency with input schema.The input schema (lines 51-54) defines
voipTokenwithnullable: true, but the response schema doesn't. While this may work ifundefinedvalues are omitted during JSON serialization, addingnullable: truewould make the contract more explicit and consistent.Proposed fix
voipToken: { type: 'string', + nullable: true, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/v1/push.ts` around lines 149 - 151, The response schema currently declares voipToken as type 'string' but omits nullable: true, causing an inconsistency with the input schema; update the response schema entry for voipToken to include "nullable: true" so it matches the input contract (look for the response schema object that contains voipToken and add nullable: true to that property).
🤖 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/app/api/server/v1/push.ts`:
- Around line 149-151: The response schema currently declares voipToken as type
'string' but omits nullable: true, causing an inconsistency with the input
schema; update the response schema entry for voipToken to include "nullable:
true" so it matches the input contract (look for the response schema object that
contains voipToken and add nullable: true to that property).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 32c55b80-2a90-4f42-8c0c-76add2a0936c
📒 Files selected for processing (2)
apps/meteor/app/api/server/v1/push.tspackages/core-typings/src/IPushToken.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core-typings/src/IPushToken.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). (4)
- GitHub Check: 📦 Build Packages
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
- 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/app/api/server/v1/push.ts
🧠 Learnings (11)
📓 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/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
📚 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/app/api/server/v1/push.ts
📚 Learning: 2025-10-06T20:32:23.658Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/utilities.ts:557-573
Timestamp: 2025-10-06T20:32:23.658Z
Learning: In packages/apps-engine/tests/test-data/utilities.ts, the field name `isSubscripbedViaBundle` in the `IMarketplaceSubscriptionInfo` type should not be flagged as a typo, as it may match the upstream API's field name.
Applied to files:
apps/meteor/app/api/server/v1/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:
apps/meteor/app/api/server/v1/push.ts
📚 Learning: 2026-01-26T18:26:01.279Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38227
File: apps/meteor/app/api/server/router.ts:44-49
Timestamp: 2026-01-26T18:26:01.279Z
Learning: In apps/meteor/app/api/server/router.ts, when retrieving bodyParams and queryParams from the Hono context via c.get(), do not add defensive defaults (e.g., ?? {}). The code should fail fast if these parameters are missing, as endpoint handlers expect them to be present and breaking here helps surface parsing problems rather than hiding them.
Applied to files:
apps/meteor/app/api/server/v1/push.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/app/api/server/v1/push.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-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/app/api/server/v1/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/app/api/server/v1/push.ts
🔇 Additional comments (5)
apps/meteor/app/api/server/v1/push.ts (5)
2-2: LGTM!Good alignment with core-typings for the import of
IPushTokenandIPushTokenTypes.
24-30: Requireidfor all push token registrations per linked issue requirements.The PR objectives state "Every push token must include a device identifier" and "it must be present on every push token." The current implementation keeps
idoptional and only validates its presence whenvoipTokenis provided.
51-54: LGTM!The schema correctly defines
voipTokenas an optional nullable string input field.
80-98: LGTM!The
PushTokenResulttype andcleanTokenResultfunction correctly handle the optionalvoipTokenfield.
166-187: LGTM!The endpoint handler correctly:
- Extracts
voipTokenfrom body params- Validates that
voipTokenrequires anid(per current design)- Conditionally includes
voipTokenin theregisterPushTokencallThe downstream token management and model layers properly handle
voipTokenpersistence per the relevant code snippets.
Proposed changes (including videos or screenshots)
Issue(s)
VMUX-44
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Improvements
Bug Fixes