fix(voip): backwards compatibility#7210
Conversation
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📜 Recent 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)
🧰 Additional context used📓 Path-based instructions (5)**/*.{js,jsx,ts,tsx,json}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{js,jsx,ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
app/definitions/**/*.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{js,ts,jsx,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (1)📓 Common learnings🔇 Additional comments (1)
WalkthroughThese changes implement version-gated behavior for push token registration, making the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes 🚥 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 |
diegolmello
force-pushed
the
pr/6918-rc311-compat
branch
from
07bc69e to
9450b20
Compare
diegolmello
changed the title
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
![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
🧹 Nitpick comments (1)
app/lib/services/restApi.ts (1)
1119-1121: Misleading type assertion for optionalid.The cast
data as TRegisterPushTokenData & { id: string }claimsidis always a string, but whenserverVersion < 8.0.0the branch at Line 1104 is skipped anddata.idisundefined. The cast hides this from the compiler rather than reflecting reality. Consider removing the cast (the server endpoint should accept the payload with or withoutidper the version-gating logic) and letting the optional type flow through, or narrowsdk.post's expected shape instead.♻️ Suggested change
- await sdk.post('push.token', data as TRegisterPushTokenData & { id: string }); + await sdk.post('push.token', data);If
sdk.post's typing forcesidto be required, the better fix is to update the endpoint's type definition to makeidoptional rather than asserting at the call site.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/services/restApi.ts` around lines 1119 - 1121, The call to sdk.post currently asserts data as TRegisterPushTokenData & { id: string } even though data.id can be undefined when serverVersion < 8.0.0; remove the misleading cast and allow the optional id to flow (call sdk.post('push.token', data) with data typed as TRegisterPushTokenData), or if sdk.post's signature requires id, update the endpoint type to make id optional instead; locate the call to sdk.post('push.token', ...) and the TRegisterPushTokenData type to apply the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/lib/services/restApi.test.ts`:
- Around line 42-45: The function declaration loadRegisterPushToken currently
splits parameters across multiple lines which violates the Prettier/ESLint
formatting rule (130-char width); fix it by placing both parameters and their
default values on a single line (e.g., loadRegisterPushToken(platform: 'ios' |
'android' = 'android', mockServerVersion = '8.0.0') ) so the signature fits
within the configured line length and adheres to the project's Prettier
settings; update any related tests/refs if they rely on formatting.
In `@ios/Libraries/VoipService.swift`:
- Around line 497-509: The timeout closure wrongly captures the Bool flag
finishAcceptInvoked by value, so it always sees the initial value and cannot
prevent double-invocation; remove finishAcceptInvoked from the timeout
DispatchWorkItem capture list (leave [weak payload] only) so the closure reads
the shared finishAcceptInvoked variable by reference like finishAccept does,
retain the existing nativeAcceptHandledCallIds check and calls to
finishAccept(false) inside timeoutWorkItem, and ensure you adjust the
timeoutWorkItem creation (symbol: timeoutWorkItem) to no longer include
finishAcceptInvoked in its capture list while leaving finishAccept and
clearNativeAcceptDedupe/ nativeAcceptHandledCallIds logic unchanged.
---
Nitpick comments:
In `@app/lib/services/restApi.ts`:
- Around line 1119-1121: The call to sdk.post currently asserts data as
TRegisterPushTokenData & { id: string } even though data.id can be undefined
when serverVersion < 8.0.0; remove the misleading cast and allow the optional id
to flow (call sdk.post('push.token', data) with data typed as
TRegisterPushTokenData), or if sdk.post's signature requires id, update the
endpoint type to make id optional instead; locate the call to
sdk.post('push.token', ...) and the TRegisterPushTokenData type to apply the
fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 68100f05-b07d-4875-b492-cf2289ff74cb
📒 Files selected for processing (4)
android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfNotification.ktapp/lib/services/restApi.test.tsapp/lib/services/restApi.tsios/Libraries/VoipService.swift
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,ts,tsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Configure Prettier with tabs, single quotes, 130 character width, no trailing commas, arrow parens avoid, and bracket same line
Files:
app/lib/services/restApi.tsapp/lib/services/restApi.test.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint with
@rocket.chat/eslint-configbase configuration including React, React Native, TypeScript, and Jest plugins
Files:
app/lib/services/restApi.tsapp/lib/services/restApi.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use TypeScript with strict mode enabled and configure baseUrl to app/ for import resolution
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbers
Files:
app/lib/services/restApi.tsapp/lib/services/restApi.test.ts
app/lib/services/restApi.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use REST API via fetch in app/lib/services/restApi.ts for HTTP requests
Files:
app/lib/services/restApi.ts
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/lib/services/restApi.tsapp/lib/services/restApi.test.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: RocketChat/Rocket.Chat.ReactNative PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-07T17:49:17.538Z
Learning: Applies to app/lib/services/voip/**/*.{ts,tsx} : Implement VoIP with WebRTC peer-to-peer audio calls in app/lib/services/voip/ using Zustand stores instead of Redux, with native CallKit (iOS) and Telecom (Android) integration; keep VoIP and VideoConf separate
📚 Learning: 2026-04-07T17:49:17.538Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.ReactNative PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-07T17:49:17.538Z
Learning: Applies to app/lib/services/voip/**/*.{ts,tsx} : Implement VoIP with WebRTC peer-to-peer audio calls in app/lib/services/voip/ using Zustand stores instead of Redux, with native CallKit (iOS) and Telecom (Android) integration; keep VoIP and VideoConf separate
Applied to files:
ios/Libraries/VoipService.swift
📚 Learning: 2026-03-30T15:49:30.957Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6875
File: app/containers/RoomItem/Actions.tsx:12-12
Timestamp: 2026-03-30T15:49:30.957Z
Learning: In RocketChat/Rocket.Chat.ReactNative, `react-native-worklets` version 0.6.1 does NOT export a built-in Jest mock (e.g., no `react-native-worklets/lib/module/mock`). The correct Jest mock approach for this version is to add a manual mock in `jest.setup.js`: `jest.mock('react-native-worklets', () => ({ scheduleOnRN: jest.fn((fn, ...args) => fn(...args)) }))`.
Applied to files:
app/lib/services/restApi.test.ts
🪛 ESLint
app/lib/services/restApi.test.ts
[error] 42-45: Replace ⏎↹platform:·'ios'·|·'android'·=·'android',⏎↹mockServerVersion·=·'8.0.0'⏎ with platform:·'ios'·|·'android'·=·'android',·mockServerVersion·=·'8.0.0'
(prettier/prettier)
🪛 GitHub Check: ESLint and Test / run-eslint-and-test
app/lib/services/restApi.test.ts
[failure] 42-42:
Replace ⏎↹platform:·'ios'·|·'android'·=·'android',⏎↹mockServerVersion·=·'8.0.0'⏎ with platform:·'ios'·|·'android'·=·'android',·mockServerVersion·=·'8.0.0'
🔇 Additional comments (1)
android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfNotification.kt (1)
115-115: LGTM!Intent extra key rename to
callermatches the key consumed byNotificationIntentHandler, resolving the empty caller name on tap.
diegolmello
had a problem deploying
to
official_ios_build
GitHub Actions
Error
diegolmello
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
diegolmello
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
diegolmello
had a problem deploying
to
official_android_build
GitHub Actions
Error
diegolmello
force-pushed
the
pr/6918-rc311-compat
branch
from
9450b20 to
a72afd5
Compare
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/lib/services/restApi.ts (1)
1075-1134:⚠️ Potential issue | 🟠 MajorDeduplicate against the version-gated payload, not the raw tokens.
Line 1094 can return early after an older/incomplete payload was registered, because Line 1123 caches the raw
voipTokeneven when it was intentionally not sent. If the server version is later populated or changes from<8.0/<8.4to a supported version in the same session, the app never posts the newly allowedid/voipToken.🐛 Proposed fix
-let lastToken = ''; -let lastVoipToken = ''; +let lastRegistrationKey = ''; type TRegisterPushTokenData = { id?: string; value: string; @@ - if (token === lastToken && voipToken === lastVoipToken) { - return; - } - const serverVersion = reduxStore.getState().server.version; - let data: TRegisterPushTokenData = { - value: '', - type: '', - appName: getBundleId - }; - if (compareServerVersion(serverVersion, 'greaterThanOrEqualTo', '8.0.0')) { - data.id = await getUniqueId(); - } - if (token) { - const type = isIOS ? 'apn' : 'gcm'; - data = { - ...data, - value: token, - type - }; - } - if (voipToken && compareServerVersion(serverVersion, 'greaterThanOrEqualTo', '8.4.0')) { - data.voipToken = voipToken; - } + const supportsPushTokenId = compareServerVersion(serverVersion, 'greaterThanOrEqualTo', '8.0.0'); + const supportsVoipToken = compareServerVersion(serverVersion, 'greaterThanOrEqualTo', '8.4.0'); + const id = supportsPushTokenId ? await getUniqueId() : undefined; + const type = isIOS ? 'apn' : 'gcm'; + const data: TRegisterPushTokenData = { + ...(id && { id }), + value: token, + type, + appName: getBundleId, + ...(voipToken && supportsVoipToken && { voipToken }) + }; + const registrationKey = JSON.stringify(data); + + if (registrationKey === lastRegistrationKey) { + return; + } try { // RC 0.60.0 - await sdk.post('push.token', data as TRegisterPushTokenData & { id: string }); - lastToken = token; - lastVoipToken = voipToken; + await sdk.post('push.token', data); + lastRegistrationKey = registrationKey; } catch (e) { log(e); } @@ if (token) { - lastToken = ''; - lastVoipToken = ''; + lastRegistrationKey = '';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/services/restApi.ts` around lines 1075 - 1134, The dedupe logic in registerPushToken compares raw tokens (lastToken/lastVoipToken) instead of the actual version-gated payload so an earlier incomplete payload can block a later valid one; change the check to compute the final payload object used for the POST (the data that includes id and/or voipToken only when server version allows) and compare that payload (e.g., JSON.stringify(data)) to a new cached lastSentPayload variable before returning early, and only update lastSentPayload after a successful sdk.post; remove or stop relying on lastToken/lastVoipToken for deduplication (you can keep them for other uses) and ensure the payload includes getBundleId and getUniqueId exactly as currently constructed so the same symbols (registerPushToken, sdk.post, compareServerVersion) are used to locate the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/lib/services/restApi.test.ts`:
- Around line 42-52: Create a regression test that exercises the same-module
version-transition cache path by calling registerPushToken twice on the same
loaded module: first load the module with loadRegisterPushToken (but do NOT call
jest.resetModules between posts) and set mockServerVersion to a pre-8.0 value
(e.g., '7.9.0'), call registerPushToken to post the token, then update the
auxStore mock to return '8.4.0' for server.version (and set Platform.OS if
needed) and call registerPushToken again on the same module instance; assert
that the second call re-posts the cached token with the new id/voipToken fields.
Use the existing loadRegisterPushToken helper, the registerPushToken function
under test, and the mocked ../store/auxStore to change versions in-process so
you cover the cache path that is missed when the module is reloaded per-version.
---
Outside diff comments:
In `@app/lib/services/restApi.ts`:
- Around line 1075-1134: The dedupe logic in registerPushToken compares raw
tokens (lastToken/lastVoipToken) instead of the actual version-gated payload so
an earlier incomplete payload can block a later valid one; change the check to
compute the final payload object used for the POST (the data that includes id
and/or voipToken only when server version allows) and compare that payload
(e.g., JSON.stringify(data)) to a new cached lastSentPayload variable before
returning early, and only update lastSentPayload after a successful sdk.post;
remove or stop relying on lastToken/lastVoipToken for deduplication (you can
keep them for other uses) and ensure the payload includes getBundleId and
getUniqueId exactly as currently constructed so the same symbols
(registerPushToken, sdk.post, compareServerVersion) are used to locate the fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cee9e04a-bafd-4714-a0a7-4440e2294f95
📒 Files selected for processing (3)
android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfNotification.ktapp/lib/services/restApi.test.tsapp/lib/services/restApi.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfNotification.kt
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,ts,tsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Configure Prettier with tabs, single quotes, 130 character width, no trailing commas, arrow parens avoid, and bracket same line
Files:
app/lib/services/restApi.test.tsapp/lib/services/restApi.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint with
@rocket.chat/eslint-configbase configuration including React, React Native, TypeScript, and Jest plugins
Files:
app/lib/services/restApi.test.tsapp/lib/services/restApi.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use TypeScript with strict mode enabled and configure baseUrl to app/ for import resolution
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbers
Files:
app/lib/services/restApi.test.tsapp/lib/services/restApi.ts
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/lib/services/restApi.test.tsapp/lib/services/restApi.ts
app/lib/services/restApi.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use REST API via fetch in app/lib/services/restApi.ts for HTTP requests
Files:
app/lib/services/restApi.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: RocketChat/Rocket.Chat.ReactNative PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-07T17:49:17.538Z
Learning: Applies to app/lib/services/voip/**/*.{ts,tsx} : Implement VoIP with WebRTC peer-to-peer audio calls in app/lib/services/voip/ using Zustand stores instead of Redux, with native CallKit (iOS) and Telecom (Android) integration; keep VoIP and VideoConf separate
📚 Learning: 2026-03-30T15:49:30.957Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6875
File: app/containers/RoomItem/Actions.tsx:12-12
Timestamp: 2026-03-30T15:49:30.957Z
Learning: In RocketChat/Rocket.Chat.ReactNative, `react-native-worklets` version 0.6.1 does NOT export a built-in Jest mock (e.g., no `react-native-worklets/lib/module/mock`). The correct Jest mock approach for this version is to add a manual mock in `jest.setup.js`: `jest.mock('react-native-worklets', () => ({ scheduleOnRN: jest.fn((fn, ...args) => fn(...args)) }))`.
Applied to files:
app/lib/services/restApi.test.ts
diegolmello
had a problem deploying
to
official_android_build
GitHub Actions
Error
diegolmello
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
diegolmello
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
diegolmello
had a problem deploying
to
official_ios_build
GitHub Actions
Error
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/lib/services/restApi.ts (1)
1098-1121: Version gating looks correct; the& { id: string }cast is a small white lie.The conditional assignment of
id(≥8.0) andvoipToken(≥8.4) matches the PR intent for RC 3.11 compatibility. The cast at Line 1121 (data as TRegisterPushTokenData & { id: string }) is used only to satisfy thepush.tokenendpoint's param type, but at runtimeidmay beundefinedfor pre-8.0 servers. Consider relaxing the endpoint definition'sidto optional so the cast can be dropped, rather than asserting a shape the code intentionally violates.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/services/restApi.ts` around lines 1098 - 1121, The code casts data to TRegisterPushTokenData & { id: string } before calling sdk.post('push.token', ...) even though id is conditionally added only for servers >=8.0 via getUniqueId(); instead, update the push.token parameter type to make id optional (e.g., id?: string) in the endpoint/type definition so the unsafe cast can be removed, and then call sdk.post('push.token', data) using the true TRegisterPushTokenData shape; locate references around serverVersion, compareServerVersion, getUniqueId, data, voipToken and sdk.post to apply the type change and remove the cast.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/lib/services/restApi.ts`:
- Around line 1098-1121: The code casts data to TRegisterPushTokenData & { id:
string } before calling sdk.post('push.token', ...) even though id is
conditionally added only for servers >=8.0 via getUniqueId(); instead, update
the push.token parameter type to make id optional (e.g., id?: string) in the
endpoint/type definition so the unsafe cast can be removed, and then call
sdk.post('push.token', data) using the true TRegisterPushTokenData shape; locate
references around serverVersion, compareServerVersion, getUniqueId, data,
voipToken and sdk.post to apply the type change and remove the cast.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5a967bdb-be1e-417f-a977-9e32df2f04a1
📒 Files selected for processing (3)
android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfNotification.ktapp/lib/services/restApi.test.tsapp/lib/services/restApi.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfNotification.kt
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,ts,tsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Configure Prettier with tabs, single quotes, 130 character width, no trailing commas, arrow parens avoid, and bracket same line
Files:
app/lib/services/restApi.test.tsapp/lib/services/restApi.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint with
@rocket.chat/eslint-configbase configuration including React, React Native, TypeScript, and Jest plugins
Files:
app/lib/services/restApi.test.tsapp/lib/services/restApi.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use TypeScript with strict mode enabled and configure baseUrl to app/ for import resolution
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbers
Files:
app/lib/services/restApi.test.tsapp/lib/services/restApi.ts
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/lib/services/restApi.test.tsapp/lib/services/restApi.ts
app/lib/services/restApi.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use REST API via fetch in app/lib/services/restApi.ts for HTTP requests
Files:
app/lib/services/restApi.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: RocketChat/Rocket.Chat.ReactNative PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-07T17:49:17.538Z
Learning: Applies to app/lib/services/voip/**/*.{ts,tsx} : Implement VoIP with WebRTC peer-to-peer audio calls in app/lib/services/voip/ using Zustand stores instead of Redux, with native CallKit (iOS) and Telecom (Android) integration; keep VoIP and VideoConf separate
📚 Learning: 2026-03-30T15:49:30.957Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6875
File: app/containers/RoomItem/Actions.tsx:12-12
Timestamp: 2026-03-30T15:49:30.957Z
Learning: In RocketChat/Rocket.Chat.ReactNative, `react-native-worklets` version 0.6.1 does NOT export a built-in Jest mock (e.g., no `react-native-worklets/lib/module/mock`). The correct Jest mock approach for this version is to add a manual mock in `jest.setup.js`: `jest.mock('react-native-worklets', () => ({ scheduleOnRN: jest.fn((fn, ...args) => fn(...args)) }))`.
Applied to files:
app/lib/services/restApi.test.ts
🔇 Additional comments (3)
app/lib/services/restApi.ts (1)
1094-1096: Dedup cache can mask a needed re-registration after a server upgrade.If the app registers on a <8.0 server (no
id, novoipToken), then the same server is upgraded to ≥8.4 withouttoken/voipTokenchanging, the early-return at Line 1094 prevents the richer payload from ever being sent. This was already raised in a prior review round; flagging again as it still applies to the current logic.app/lib/services/restApi.test.ts (2)
188-243: New version-gating cases cover the PR intent; one gap remains.The
<8.0,8.0–8.3, and≥8.4cases correctly assert the request-shape contract. However, the same-module version-transition regression (e.g., initial post on<8.0followed by re-post on≥8.4without token changes) is still missing — each test reloads the module, so it never exercises the dedup cache path. Previously flagged; reiterating since the latest commit didn't add that case.
42-66: Test helper changes LGTM.Mocking
../store/auxStoreviajest.doMockwith a parameterizedmockServerVersionis the right approach for per-test version gating, andjest.resetModules()ensuresrestApi.tspicks up the mock freshly.
diegolmello
force-pushed
the
pr/6918-rc311-compat
branch
from
a72afd5 to
442d1e7
Compare
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
diegolmello
had a problem deploying
to
official_ios_build
GitHub Actions
Error
diegolmello
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
diegolmello
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
diegolmello
had a problem deploying
to
official_android_build
GitHub Actions
Error
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/lib/services/restApi.ts (1)
1119-1121: Misleading type assertion contradicts the version gating.The cast
data as TRegisterPushTokenData & { id: string }assertsidis always present, but the whole point of the change is thatidis omitted for servers < 8.0.0. This defeats the optional typing introduced on line 1079 and can hide real bugs if the endpoint's type signature is later tightened.Consider loosening the
sdk.postendpoint type (or its payload type) soidis optional on the request, and drop the cast here.🔧 Suggested change
- await sdk.post('push.token', data as TRegisterPushTokenData & { id: string }); + await sdk.post('push.token', data);(This requires the
push.tokenendpoint typing to acceptidas optional; update the endpoint definition accordingly.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/services/restApi.ts` around lines 1119 - 1121, The cast forcing an id on the payload (data as TRegisterPushTokenData & { id: string }) contradicts the version gating and optional id introduced earlier; remove the inline assertion and update the push.token endpoint typing so its request payload accepts id as optional (or loosen sdk.post’s generic for 'push.token') so sdk.post('push.token', data) can be called without forcing id; locate the call to sdk.post in restApi.ts and adjust the endpoint/type definition for 'push.token' (and any related TRegisterPushTokenData declaration) so id is optional rather than asserting it here.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt`:
- Around line 313-340: The null branch in answerIncomingCall creates a synthetic
VoipPayload with empty host, calls cancelById(appCtx, 0), and redundantly
invokes ddpRegistry.stopClient and context.applicationContext, which prevents JS
from dispatching a failure; change answerIncomingCall to return a Boolean
(success/failure) or accept the real VoipPayload so failure handling is
centralized in finish(), remove the redundant ddpRegistry.stopClient call and
the unnecessary applicationContext usage, and call cancelById using the real
payload.notificationId instead of 0, ensuring you call
VoipModule.storeAcceptFailureForJs with the original VoipPayload (including
host) so MediaCallEvents.ts can detect and dispatch the failure.
---
Nitpick comments:
In `@app/lib/services/restApi.ts`:
- Around line 1119-1121: The cast forcing an id on the payload (data as
TRegisterPushTokenData & { id: string }) contradicts the version gating and
optional id introduced earlier; remove the inline assertion and update the
push.token endpoint typing so its request payload accepts id as optional (or
loosen sdk.post’s generic for 'push.token') so sdk.post('push.token', data) can
be called without forcing id; locate the call to sdk.post in restApi.ts and
adjust the endpoint/type definition for 'push.token' (and any related
TRegisterPushTokenData declaration) so id is optional rather than asserting it
here.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 419e3fd2-c97d-4d82-86b1-9d1cb215cdb4
📒 Files selected for processing (6)
android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfNotification.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.ktapp/containers/NewMediaCall/VoipCallLifecycle.integration.test.tsxapp/lib/services/restApi.test.tsapp/lib/services/restApi.tsapp/lib/services/voip/MediaSessionInstance.ts
✅ Files skipped from review due to trivial changes (1)
- app/lib/services/voip/MediaSessionInstance.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/lib/services/restApi.test.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: ESLint and Test / run-eslint-and-test
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{js,jsx,ts,tsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Configure Prettier with tabs, single quotes, 130 character width, no trailing commas, arrow parens avoid, and bracket same line
Files:
app/containers/NewMediaCall/VoipCallLifecycle.integration.test.tsxapp/lib/services/restApi.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint with
@rocket.chat/eslint-configbase configuration including React, React Native, TypeScript, and Jest plugins
Files:
app/containers/NewMediaCall/VoipCallLifecycle.integration.test.tsxapp/lib/services/restApi.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use TypeScript with strict mode enabled and configure baseUrl to app/ for import resolution
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbers
Files:
app/containers/NewMediaCall/VoipCallLifecycle.integration.test.tsxapp/lib/services/restApi.ts
app/containers/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Place reusable UI components in app/containers/ directory
Files:
app/containers/NewMediaCall/VoipCallLifecycle.integration.test.tsx
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/containers/NewMediaCall/VoipCallLifecycle.integration.test.tsxapp/lib/services/restApi.ts
app/lib/services/restApi.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use REST API via fetch in app/lib/services/restApi.ts for HTTP requests
Files:
app/lib/services/restApi.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: RocketChat/Rocket.Chat.ReactNative PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-07T17:49:17.538Z
Learning: Applies to app/lib/services/voip/**/*.{ts,tsx} : Implement VoIP with WebRTC peer-to-peer audio calls in app/lib/services/voip/ using Zustand stores instead of Redux, with native CallKit (iOS) and Telecom (Android) integration; keep VoIP and VideoConf separate
📚 Learning: 2026-04-07T17:49:17.538Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.ReactNative PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-07T17:49:17.538Z
Learning: Applies to app/lib/services/voip/**/*.{ts,tsx} : Implement VoIP with WebRTC peer-to-peer audio calls in app/lib/services/voip/ using Zustand stores instead of Redux, with native CallKit (iOS) and Telecom (Android) integration; keep VoIP and VideoConf separate
Applied to files:
app/containers/NewMediaCall/VoipCallLifecycle.integration.test.tsxandroid/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt
🔇 Additional comments (3)
android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfNotification.kt (1)
115-115: Key rename aligns with NotificationIntentHandler.Switching the extra key to
"caller"matches the consumer side and fixes the empty caller name on notification taps. Looks good.app/lib/services/restApi.ts (1)
1098-1117: Version gating logic LGTM.The
>= 8.0.0gate foridand>= 8.4.0gate forvoipTokencorrectly restore backwards compatibility with pre-8.0 / pre-VoIP servers, consistent with the pattern used elsewhere in this file.app/containers/NewMediaCall/VoipCallLifecycle.integration.test.tsx (1)
345-355: Test updates correctly track the new warning string.Both the allowlist entry and the A2
toHaveBeenCalledWithassertion are synchronized with the renamed warning inMediaSessionInstance.ts. Asserting the exact string ('[VoIP] Call not found after accept:', 'missing-1') also guards against future silent regressions if the message is rephrased without updating this test.Also applies to: 589-589
…deoConf caller extra Gate push.token id field behind RC >= 8.0.0 check to avoid silent registration failures on older servers. Gate voipToken field behind RC >= 8.4.0 check since pre-VoIP servers may reject the unknown field. Fix VideoConfNotification to send "caller" instead of "callerName" in intent extras, matching NotificationIntentHandler's expected key. Co-authored-by: Claude <claude@anthropic.com>
diegolmello
force-pushed
the
pr/6918-rc311-compat
branch
from
442d1e7 to
9c13144
Compare
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Failure
diegolmello
had a problem deploying
to
experimental_android_build
GitHub Actions
Failure
diegolmello
had a problem deploying
to
official_ios_build
GitHub Actions
Failure
diegolmello
had a problem deploying
to
official_android_build
GitHub Actions
Failure
diegolmello
had a problem deploying
to
experimental_ios_build
GitHub Actions
Failure
…/Decline (#7215) * merge feat.voip-lib * feat(voip): enhance call handling with UUID mapping and event listeners * Base call UI * feat(voip): integrate Zustand for call state management and enhance CallView UI * feat(voip): add simulateCall function for mock call handling in UI development * refactor(CallView): update button handlers and improve UI responsiveness * Add pause-shape-unfilled icon * Base CallHeader * toggleFocus * collapse buttons * Header components * Hide header when no call * Timer * Add use memo * Add voice call item on sidebar * cleanup * Temp use @rocket.chat/media-signaling from .tgz * cleanup * Check module and permissions to enable voip * Refactor stop method to use optional chaining for media signal listeners * voip push first test * Add VoIP call handling with pending call management - Implemented VoIP push notification handling in index.js, including storing call info for later processing. - Added CallKeep event handlers for answering and ending calls from a cold start. - Introduced a new CallIdUUID module to convert call IDs to deterministic UUIDs for compatibility with CallKit. - Created a pending call store to manage incoming calls when the app is not fully initialized. - Updated deep linking actions to include VoIP call handling. - Enhanced MediaSessionInstance to process pending calls and manage call states effectively. * Remove pending store and create getInitialEvents on app/index * Attempt to make iOS calls work from cold state * lint and format * Patch callkeep ios * Temp send iOS voip push token on gcm * Temp fix require cycle * chore: format code and fix lint issues [skip ci] * CallIDUUID module on android and voip push * Add setCallUUID on useCallStore to persist calls accepted on native Android * remove callkeep from notification * Android Incoming Call UI POC * Refactor VoIP handling: Migrate VoIP-related classes to a new package structure, removing deprecated modules and consolidating functionality. Update imports in MainApplication and NotificationIntentHandler to reflect changes. This cleanup enhances code organization and prepares for future VoIP feature enhancements. * Remove VoipForegroundService * cleanup and use caller instead of callerName * Cleanup and make iOS build again * Refactor VoIP handling: Remove unused event emissions for call answered and declined, switch from SharedPreferences to in-memory storage for pending VoIP call data, and update method signatures for better clarity. This cleanup enhances performance and prepares for future VoIP feature improvements. * Refactor VoIP handling: Introduce a new VoipPayload class to encapsulate call data, streamline notification processing, and enhance method signatures across the VoIP module. This update improves code clarity and prepares for future feature enhancements. * Migrate react-native-voip-push-notifications to VoipModule * Refactor VoIP module: Update package structure by moving VoipTurboPackage to the main package and removing the obsolete NativeVoipSpec class. Adjust imports in MainApplication and VoipModule to reflect these changes, enhancing code organization and maintainability. * Unify emitters * Move CallKeep listeners from MediaSessionInstance to getInitialEvents * Clear callkeep on endcall * Unify getInitialEvents logic * getInitialEvents -> MediaCallEvents * chore: format code and fix lint issues [skip ci] * feat(Android): Add full screen incoming call (#6977) * feat: Update call UI (#6990) * feat: Handle audio routing, e.g., Bluetooth headset vs. internal speaker switching (#6992) * fix: empty space when not on call (#6993) * feat: Dialpad (#7000) * action: organized translations * feat: start call (#7024) * chore: format code and fix lint issues * feat: Pre flight (#7038) * action: organized translations * feat: Receive voip push notifications from backend (#7045) * feat: Refactor media session handling and improve disconnect logic (#7065) * feat: Control incoming call from native (#7066) * feat: Voice message blocks (#7057) * feat: native accept success event (#7068) * feat(voip): call waiting, busy detection, and videoconf blocking (#7077) * action: organized translations * feat(voip): tap-to-hide call controls with animations (#7078) * feat(voip): navigate to call DM from message button and header (#7082) * feat(voip): tablet and landscape layout (#7110) * chore: develop into feat.voip-lib-new (RN 81 + Expo 54 + reanimated 4 + true-sheet + iOS 26) (#7114) * chore: format code and fix lint issues * feat(voip): android landscape layout for IncomingCallActivity (#7116) * Update agents files * feat(voip): Support a11y (#7106) * Fix content cutting on iOS on some edge cases * pods * Ignore .worktrees on jest * chore: Merge develop into feat.voip-lib-new (#7129) * fix(voip): show CallKit UI when call is active in background (#7128) * chore: Update media-signaling to 0.2.0 (#7153) * feat(voip): migrate iOS accept/reject from DDP to REST (#7124) * Fix icons * feat(voip): migrate Android accept/reject from DDP to REST (#7127) * test(voip): integration tests for CallView pipeline (#7161) * feat(voip): display video conf provider as subtitle (#7160) * fix(voip): CallView button grid and correct landscape/dialpad layouts (#7164) * fix(voip): prevent stale MMKV cache on Android first-install accept MMKVKeyManager.initialize ran in MainApplication.onCreate before the JS engine started and opened the default MMKV file via the Tencent 1.2 JAR when it was still empty. Tencent caches instances per-ID in a singleton registry, so that empty-state view was held for the rest of the process. JS later wrote credentials through react-native-mmkv (MMKV Core 2.0), which has its own separate registry. When a VoIP push arrived, Ejson.getMMKV() got the cached empty Tencent instance and reported "No userId found in MMKV for server". Closing and reopening the app cleared the cache, which is why only the very first call after install failed. Drop the open/verify block — the encryption key is already cached from SecureKeystore, so no MMKV handle is needed here. The first Tencent instance is now created inside Ejson.getMMKV() after JS has written, so it scans the file fresh. * fix(voip): prevent duplicate ringtone on Android incoming call (#7158) * fix(voip): set explicit snaps for NewMediaCall bottom sheet (#7165) * Update app/lib/services/voip/MediaSessionStore.ts Co-authored-by: Pierre Lehnen <55164754+pierre-lehnen-rc@users.noreply.github.com> * fix: make startVoipFork reactive to permissions-changed (#7151) * fix(android): remove MediaProjectionService from merged manifest (#7190) * fix(voip): Phone account creation (#7170) * feat: add Enable Mobile Ringing toggle in user preferences (#7155) * fix(voip): ship blockers for PushKit, licensing, outbound calls, push tokens (#7167) * fix(android): Play Store mic discoverability, safer FCM logs, avatar auth via headers (#7171) * fix(ios): serialize VoipService bridge statics (#7169) * fix(voip): Android DDP thread safety and VoipPayload bundle parity (#7168) * chore(voip): dead-code and hygiene sweep (#7174) * refactor(voip): decouple navigateToCallRoom from Redux and backfill REST/connect tests (#7176) * test(voip): tighten ringing endCall assertion and add VideoConf VoIP-lock saga coverage (#7177) * fix(ios): harden VoIP DDP WebSocket client on receive failures and TLS (#7173) * refactor(voip): MediaCallEvents Redux adapters and resetVoipState (#7178) * refactor(voip): decouple peer autocomplete from Redux; simplify NewMediaCall (#7175) * fix(ios): add NS_SWIFT_NAME to Challenge.runChallenge for Swift 6.2 compatibility Swift 6.2 (Xcode 26.x / macos-26 runner) auto-renames the Objective-C method runChallenge:didReceiveChallenge:completionHandler: to run(_:didReceive:completionHandler:) when imported into Swift. Add NS_SWIFT_NAME to explicitly pin the Swift import name, preventing the compiler from applying its heuristics. This keeps the existing Swift call site in DDPClient.swift working without changes. * fix(ios): cancel old URLSession/webSocketTask before reconnecting in DDPClient.connect (#7197) * fix(ios): add NSLock to nativeAcceptHandledCallIds and 10s REST timeout to handleNativeAccept (#7198) * feat(android): create VoipCallService with FOREGROUND_SERVICE_MICROPHONE (#7199) * fix(android): start VoipCallService on accept, stop on hangup/timeout, install end-call listener (#7200) * fix(voip): enable DM nav for users with SIP extension (#7203) * fix(android): handle null VoiceConnection in answerIncomingCall, notify JS (#7201) * fix(voip): resolve closure capture ordering in handleNativeAccept (#7209) * fix(android): integrate VoIP modules with SSL-pinned OkHttpClient (#7208) * fix(push): gate id and voipToken behind server version checks, fix VideoConf caller extra (#7210) * fix(voip): remove sensitive data from production logs (#7207) * fix(android): remove isRunning guard + add double-tap guard on Accept/Decline - VoipCallService: remove if (!isRunning) guard, call startForeground unconditionally (idempotent on Android, fixes Android 14+ foreground service requirement) - IncomingCallActivity: add AtomicBoolean guard on handleAccept/handleDecline to prevent double-tap from triggering multiple service starts --------- Co-authored-by: diegolmello <diegolmello@users.noreply.github.com> Co-authored-by: Pierre Lehnen <55164754+pierre-lehnen-rc@users.noreply.github.com>
1 participant
Summary
Fix 3 regressions introduced by PR #6918 (feat: Voice support / VoIP) that break non-VoIP functionality on Rocket.Chat servers 3.11 and earlier.
Changes
Gate push.token
idfield behind RC >= 8.0.0registerPushTokeninrestApi.tsconditionally sends theidfield only whencompareServerVersion(serverVersion, 'greaterThanOrEqualTo', '8.0.0')is true. Pre-8.0 servers may reject the unknown field and silently fail token registration.Fix VideoConf caller intent extra name mismatch
VideoConfNotification.ktchangedputString("callerName", callerName)toputString("caller", callerName). This aligns withNotificationIntentHandlerwhich readscaller, fixing VideoConf notification taps that were receiving an empty caller name.Gate
voipTokenbehind RC >= 8.4.0voipTokenfield is now only sent whencompareServerVersion(serverVersion, 'greaterThanOrEqualTo', '8.4.0')is true. Pre-VoIP servers may reject the unknown field.Issue(s)
How to test or reproduce
Run the
registerPushTokentests:TZ=UTC yarn test app/lib/services/restApi.test.tsTypes of changes
Checklist
Further comments
This PR must be merged after the base branch
feat.voip-lib-new(PR #6918).Related: #6918
Summary by CodeRabbit
Release Notes
Improvements
Tests