feat: Voice support #6918
feat: Voice support #6918
Conversation
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
WalkthroughAdds end-to-end VoIP: native iOS CallIdUUID and PushKit wiring, Android manifest services, RNCallKeep integration, WebRTC/media-signaling packages, a Zustand call store and media session manager, UI (CallHeader, CallView + components), stories/tests/mocks, ICE parsing/utilities, saga/deep-link handling, and related package/patch updates. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LoginSaga
participant MediaSessionInstance
participant RNCallKeep
participant Server
participant CallView
User->>LoginSaga: login success
LoginSaga->>MediaSessionInstance: initialize & setup CallKeep
MediaSessionInstance->>RNCallKeep: setup listeners / register
MediaSessionInstance->>Server: subscribe to media-signal
Server-->>MediaSessionInstance: media-signal incoming (call)
MediaSessionInstance->>RNCallKeep: displayIncomingCall(callUUID,...)
RNCallKeep-->>User: incoming call UI
User->>RNCallKeep: answer
RNCallKeep->>MediaSessionInstance: answer callback
MediaSessionInstance->>CallView: navigate + setCall
User->>CallView: toggle mute/hold/speaker
CallView->>MediaSessionInstance: send media-signal action -> Server
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/AppContainer.tsx (1)
39-46: Guard against undefinedcurrentRouteName.
getActiveRouteName(state)can returnundefinedif the navigation state is empty or unavailable. PassingundefinedtosetCurrentScreenmay cause issues with analytics logging.useEffect(() => { if (root) { const state = Navigation.navigationRef.current?.getRootState(); const currentRouteName = getActiveRouteName(state); - Navigation.routeNameRef.current = currentRouteName; - setCurrentScreen(currentRouteName); + if (currentRouteName) { + Navigation.routeNameRef.current = currentRouteName; + setCurrentScreen(currentRouteName); + } } }, [root]);
🤖 Fix all issues with AI agents
In `@app/containers/CallHeader/components/Title.tsx`:
- Around line 39-44: The Timer is always rendered causing "Connecting00:00";
update the JSX in Title.tsx to render <Timer /> only when the header is not in
the connecting state—use the existing isConnecting flag (or the result of
getHeaderTitle() === "Connecting") to conditionally include Timer so when
isConnecting is true the Timer is omitted; locate the getHeaderTitle() call and
the Timer component in the return block and wrap Timer with a conditional check
tied to isConnecting (or title check).
In `@app/lib/services/voip/MediaSessionInstance.ts`:
- Around line 157-163: The subscription currently compares array references
(getIceServers() !== this.iceServers) which always differs because getIceServers
returns a new array; replace this with a deep/element-wise equality check (e.g.,
use a utility like lodash/isEqual or compare lengths and each entry) to detect
real changes, only assign this.iceServers and call the previously commented
this.instance?.setIceServers(this.iceServers) when the contents actually differ,
and ensure this.instance exists (guard or optional chaining) before calling
setIceServers; alternatively, modify getIceServers to return a stable/memoized
reference to avoid churn.
In `@app/lib/services/voip/useCallStore.ts`:
- Around line 59-108: The setCall function attaches listeners but never removes
them; modify setCall to store the listener functions (e.g., handleStateChange,
handleTrackStateChange, handleEnded) in the store state or a local cleanup map
keyed by callUUID and call.emitter, remove any existing listeners before
attaching new ones, and ensure you call emitter.off/removeListener for each
listener when the call ends (inside handleEnded) and when replacing/clearing the
call (e.g., in reset). Reference setCall, handleStateChange,
handleTrackStateChange, handleEnded, call.emitter.on and use
call.emitter.off/removeListener to unsubscribe so listeners are cleaned up and
stale closures are avoided.
In `@app/sagas/login.js`:
- Around line 269-278: The saga startVoipFork is calling the async
simulateCall() directly so the generator neither waits for completion nor
catches its errors; replace the direct call with the Redux-Saga effect (i.e.,
yield call(simulateCall)) so the saga will await the Promise and any exceptions
from simulateCall will be caught by the surrounding try/catch; keep the existing
initCallKeep and mediaSessionInstance.init(userId) calls as-is and use the
simulateCall symbol with call() to locate the change.
- Around line 260-263: Remove the leftover debug setInterval in the login saga:
delete the timer creation (the setInterval(...) block) and the unused start
variable so no perpetual console logging occurs; locate the code around the
start = Date.now() and the setInterval(...) in the login saga
(app/sagas/login.js) and remove those lines before merging.
In `@app/views/SidebarView/components/Stacks.tsx`:
- Around line 29-38: The onPress handler for the List.Item calls simulateCall()
(async) and then toggleFocus() synchronously, causing a race where callUUID may
not be set; change the handler to be async, await simulateCall() before calling
toggleFocus(), and handle/rethrow errors as appropriate so toggleFocus() only
runs after simulateCall (refer to the onPress callback in the List.Item, the
simulateCall function, and the toggleFocus function).
In `@package.json`:
- Line 50: The package.json currently points the dependency
"@rocket.chat/media-signaling" to a local filesystem path that breaks CI and
other developers' installs; update the dependency entry for
"@rocket.chat/media-signaling" in package.json to a proper registry or VCS
reference (for example an npm package name with a version, a GitHub URL like
"github:RocketChat/media-signaling#tag", or a git+ssh URL), replacing the
absolute local path and ensure the version/ref used is published or reachable so
installs succeed for everyone.
- Line 95: The package.json currently pins react-native-callkeep to
"react-native-callkeep": "^4.3.16", which is known to have runtime compatibility
issues with React Native 0.79; update the dependency to a version proven
compatible (or remove/replace it) and run full Android/iOS smoke tests
(including New Architecture on iOS) before merging — locate the dependency entry
"react-native-callkeep" in package.json, check upstream issues/PRs for a
recommended version or patch, change the version spec accordingly (or replace
with an alternate package), then run yarn/npm install and platform-specific
tests to verify fixes.
In `@patches/react-native-callkeep`+4.3.16.patch:
- Around line 1-98: Patch contains generated Android build artifacts
(.dex/.bin/etc.) that must be removed; regenerate the patch to include only the
source change in RNCallKeepModule.java. Remove all added files under
node_modules/react-native-callkeep/android/build/.transforms and any binary
artifacts from the patch, then create a new patch that only modifies
RNCallKeepModule.java (the meaningful change referenced at the end of the patch)
and excludes build outputs and any absolute-path metadata; verify by grepping
the patch for "RNCallKeepModule.java" and for binary extensions (.dex .class
.jar .bin) before committing.
- Around line 507-534: The call to RNCallKeep.displayIncomingCall in
MediaSessionInstance.ts uses the old 5-argument form and passes a string
'generic' where the new 4-argument signature expects a boolean hasVideo; update
the invocation in MediaSessionInstance.ts (the call with
RNCallKeep.displayIncomingCall(callUUID, displayName, displayName, 'generic',
false)) to match the new method RNCallKeep.displayIncomingCall(uuid, number,
callerName, hasVideo) by removing the extraneous string and passing a boolean
(e.g., RNCallKeep.displayIncomingCall(callUUID, displayName, displayName, false)
or a proper hasVideo variable), and then search for other usages of
RNCallKeep.displayIncomingCall to ensure no other call sites rely on the removed
3-arg overload.
🟡 Minor comments (13)
app/lib/store/index.ts-8-8 (1)
8-8: Clarify intent or re-enable the redux logger in development builds.The logger is disabled only in development builds (lines 8 and 22) with no other references or dependencies in the codebase. However, the change lacks any explanation. If intentional, add a comment documenting why; if temporary, re-enable before merging to preserve the development debugging experience.
app/views/CallView/styles.ts-6-9 (1)
6-9: Remove debug background color before merging.
backgroundColor: 'red'appears to be a debug/placeholder value. This should be replaced with the appropriate theme color or removed.🐛 Suggested fix
container: { - flex: 1, - backgroundColor: 'red' + flex: 1 },app/lib/services/voip/MediaCallLogger.ts-14-20 (1)
14-20: Use appropriate console methods for error and warning levels.The
errorandwarnmethods useconsole.log, which loses semantic meaning. This affects:
- Log level filtering in monitoring/crash reporting tools
- Developer experience (red/yellow boxes in RN dev mode)
- Log aggregation and alerting
🔧 Proposed fix
error(...args: unknown[]): void { - console.log(`[Media Call Error] ${JSON.stringify(args)}`); + console.error(`[Media Call Error] ${JSON.stringify(args)}`); } warn(...args: unknown[]): void { - console.log(`[Media Call Warning] ${JSON.stringify(args)}`); + console.warn(`[Media Call Warning] ${JSON.stringify(args)}`); }app/containers/CallHeader/components/Collapse.tsx-15-17 (1)
15-17: Accessibility label should reflect the current action.The label is always "Minimize" but the button toggles between minimize and maximize actions. Screen reader users will hear "Minimize" even when the action would expand the view.
<HeaderButton.Item - accessibilityLabel={I18n.t('Minimize')} + accessibilityLabel={focused ? I18n.t('Minimize') : I18n.t('Maximize')} onPress={toggleFocus} iconName={focused ? 'arrow-collapse' : 'arrow-expand'} color={colors.fontDefault} />Ensure the
Maximizekey exists in the i18n locales.app/views/RoomView/components/HeaderCallButton.tsx-7-14 (1)
7-14: Props type inconsistency:ridis required but unused.The
ridprop is required in the type definition (line 12) but commented out from destructuring (line 8). This forces callers to pass a value that's ignored. Either useridor make it optional/remove it.Proposed fix: remove rid from required props
export const HeaderCallButton = ({ - // rid, disabled, accessibilityLabel }: { - rid: string; disabled: boolean; accessibilityLabel: string; }): React.ReactElement | null => {app/lib/services/voip/simulateCall.ts-54-88 (1)
54-88:stateproperty won't reflect updates afterhangup/reject.The
stateproperty on line 56 is assigned once at object creation. Unlikemutedandheldwhich use getters, whenhangup()orreject()updatecurrentState, themockCall.stateproperty will still return the original value.Proposed fix: convert state to a getter
const mockCall = { callId: `mock-call-${Date.now()}`, - state: currentState, + get state() { + return currentState; + }, get muted() { return currentMuted; },app/views/CallView/index.test.tsx-126-138 (1)
126-138: Test does not verify what it claims.The test is named "should not show CallStatusText when call is not active" but only verifies that
caller-infoexists. It doesn't actually assert thatCallStatusTextis absent.💡 Suggestion
Add a
testIDtoCallStatusTextand assert its absence:- // CallStatusText should not be rendered when not active - // We can't easily test for the Text component directly, so we verify the caller-info is rendered - // but CallStatusText won't be in the tree when callState is not 'active' - expect(queryByTestId('caller-info')).toBeTruthy(); + expect(queryByTestId('caller-info')).toBeTruthy(); + expect(queryByTestId('call-status-text')).toBeNull();app/views/CallView/index.test.tsx-114-124 (1)
114-124: Fragile assertion using whitespace regex.The test
expect(getByText(/\s/)).toBeTruthy()matches any whitespace, which is not a reliable way to verifyCallStatusTextrenders. This could pass even ifCallStatusTextisn't rendered but some other element contains whitespace.💡 Suggestion
Consider adding a
testIDtoCallStatusTextand querying for it directly:- // CallStatusText should render (it shows a space when not muted/on hold) - expect(getByText(/\s/)).toBeTruthy(); + expect(getByTestId('call-status-text')).toBeTruthy();app/views/CallView/components/CallerInfo.stories.tsx-46-48 (1)
46-48:WithOnlineStatusstory is identical toDefault.Both stories render
<CallerInfo />with the same store state from the decorator. IfWithOnlineStatusis meant to demonstrate different behavior, it should set up distinct state (e.g., an online status indicator).💡 Suggestion
Either differentiate this story or remove it:
-export const WithOnlineStatus = () => <CallerInfo />; +export const WithOnlineStatus = () => { + // If there's an online status feature, set appropriate state here + setStoreState({ displayName: 'Bob Burnquist', username: 'bob.burnquist', sipExtension: '2244' }); + // Add online status setup when implemented + return <CallerInfo />; +};app/containers/CallHeader/components/Timer.tsx-20-30 (1)
20-30: Duration not reset when call ends.When
callStartTimebecomesnull(call ends or reset), the effect returns early butdurationstate retains its last value. If the component remains mounted, it will display stale duration.useEffect(() => { if (!callStartTime) { + setDuration(0); return; } const updateDuration = () => { setDuration(Math.floor((Date.now() - callStartTime) / 1000)); }; updateDuration(); const interval = setInterval(updateDuration, 1000); return () => clearInterval(interval); }, [callStartTime]);app/lib/services/voip/parseStringToIceServers.ts-18-24 (1)
18-24: Empty or whitespace-only entries produce invalid IceServer objects.When the input contains empty entries (e.g.,
"turn:a.com,,turn:b.com"or"turn:a.com, ,turn:b.com"), they pass through and createIceServerobjects with empty or whitespaceurls. Consider filtering invalid entries:export const parseStringToIceServers = (string: string): IceServer[] => { if (!string) { return []; } - const lines = string.trim() ? string.split(',') : []; - return lines.map(line => parseStringToIceServer(line)); + return string + .split(',') + .map(line => line.trim()) + .filter(line => line.length > 0) + .map(parseStringToIceServer); };app/lib/services/voip/MediaSessionInstance.ts-72-74 (1)
72-74: Remove debug console.log or replace with structured logging.This console.log statement with emoji appears to be a debug artifact. Either remove it or replace with the
MediaCallLoggerthat's already used elsewhere in this codebase.Suggested fix
call.emitter.on('stateChange', oldState => { - console.log(`📊 ${oldState} → ${call.state}`); + // Use MediaCallLogger if state transition logging is needed });app/views/CallView/CallView.stories.tsx-96-99 (1)
96-99: State mutation during render may cause issues.Calling
setStoreState()directly in the story's render function mutates global state during React's render phase. This can cause unpredictable behavior and state bleeding between stories.Consider using Storybook's
playfunction,loaders, ordecoratorsto set state before rendering:Suggested approach using decorator pattern
export const ConnectedCall = () => { - setStoreState({ callState: 'active', callStartTime: new Date().getTime() - 61000 }); return <CallView />; }; +ConnectedCall.decorators = [ + (Story: React.ComponentType) => { + React.useEffect(() => { + setStoreState({ callState: 'active', callStartTime: new Date().getTime() - 61000 }); + }, []); + return <Story />; + } +];
🧹 Nitpick comments (26)
__mocks__/react-native-callkeep.js (1)
1-10: Consider expanding mock coverage for additional CallKeep methods.The mock covers basic CallKeep functionality. Depending on test requirements, you may need to add mocks for additional commonly used methods like
startCall,reportEndCallWithUUID,setMutedCall,answerIncomingCall, orrejectCall.💡 Example expansion if needed
export default { setup: jest.fn(), canMakeMultipleCalls: jest.fn(), displayIncomingCall: jest.fn(), endCall: jest.fn(), setCurrentCallActive: jest.fn(), + startCall: jest.fn(), + answerIncomingCall: jest.fn(), + rejectCall: jest.fn(), + reportEndCallWithUUID: jest.fn(), + setMutedCall: jest.fn(), addEventListener: jest.fn((event, callback) => ({ remove: jest.fn() })) };app/views/CallView/components/CallActionButton.tsx (2)
40-52:activevariant has inconsistent non-pressed background color.When
variant='active'and not pressed,getBackgroundColorfalls through to the default case (line 51), returningbuttonBackgroundSecondaryDefault. This means an 'active' (toggled) button looks identical to a 'default' button when not pressed.Consider adding an explicit case for 'active' in the non-pressed state:
♻️ Suggested fix
- return variant === 'danger' ? colors.buttonBackgroundDangerDefault : colors.buttonBackgroundSecondaryDefault; + switch (variant) { + case 'active': + return colors.buttonBackgroundSecondaryDangerDefault; + case 'danger': + return colors.buttonBackgroundDangerDefault; + default: + return colors.buttonBackgroundSecondaryDefault; + }
54-71: Label is outside the touch target.The
Textlabel (line 69) is rendered outside thePressable, so tapping the label won't triggeronPress. Consider wrapping both the button and label inside thePressableto increase the touch target area.♻️ Optional restructure
return ( - <View> - <Pressable - onPress={onPress} - disabled={disabled} - style={({ pressed }) => [ - styles.actionButton, - disabled && { opacity: 0.5 }, - { backgroundColor: getBackgroundColor(pressed) } - ]} - accessibilityLabel={label} - accessibilityRole='button' - testID={testID}> - <CustomIcon name={icon} size={32} color={getIconColor()} /> - </Pressable> - <Text style={[styles.actionButtonLabel, { color: colors.fontDefault }]}>{label}</Text> - </View> + <Pressable + onPress={onPress} + disabled={disabled} + accessibilityLabel={label} + accessibilityRole='button' + testID={testID}> + {({ pressed }) => ( + <View style={disabled && { opacity: 0.5 }}> + <View style={[styles.actionButton, { backgroundColor: getBackgroundColor(pressed) }]}> + <CustomIcon name={icon} size={32} color={getIconColor()} /> + </View> + <Text style={[styles.actionButtonLabel, { color: colors.fontDefault }]}>{label}</Text> + </View> + )} + </Pressable> );app/views/CallView/components/CallerInfo.tsx (1)
29-31: Empty View renders for commented-out Status component.The
Statuscomponent is commented out (lines 10, 30), but the wrapperViewwithbackgroundColor: colors.surfaceHoverstill renders. This leaves an empty styled element that may appear as a visual artifact.Consider either uncommenting the Status component when ready, or removing the entire status badge View:
♻️ Remove empty View until Status is implemented
<View style={styles.avatarContainer}> - <AvatarContainer text={avatarText} size={120} borderRadius={16}> - <View style={[sharedStyles.status, { backgroundColor: colors.surfaceHover }]}> - {/* <Status size={20} id={contact.username} /> */} - </View> - </AvatarContainer> + <AvatarContainer text={avatarText} size={120} borderRadius={16} /> </View>app/lib/services/voip/MediaCallLogger.ts (1)
4-6: Consider defensive JSON.stringify handling.
JSON.stringify(args)will throw on circular references. Media signaling objects could potentially contain complex structures.♻️ Optional: Safe stringify helper
+const safeStringify = (args: unknown[]): string => { + try { + return JSON.stringify(args); + } catch { + return args.map(a => String(a)).join(', '); + } +}; + export class MediaCallLogger implements IMediaSignalLogger { log(...args: unknown[]): void { - console.log(`[Media Call] ${JSON.stringify(args)}`); + console.log(`[Media Call] ${safeStringify(args)}`); } // ... apply similarly to other methodsapp/containers/Header/components/HeaderContainer/index.tsx (2)
7-12: Remove unusedaddExtraNotchPaddingfrom interface.The
addExtraNotchPaddingprop is declared inIHeaderContainer(line 8) but is no longer destructured or used in the component (line 14). This is now dead code.♻️ Clean up interface
interface IHeaderContainer extends ViewProps { - addExtraNotchPadding?: boolean; isMasterDetail?: boolean; customLeftIcon?: boolean; customRightIcon?: boolean; }
14-15: Redundant memoization: bothmemo()and'use memo'directive used.The component uses both the
memo()HOC wrapper (line 14) and the React Compiler's'use memo'directive (line 15). These serve the same purpose and using both is redundant.Choose one approach:
- Keep
memo()HOC (explicit, works without compiler) and remove'use memo'- Or remove
memo()and rely on'use memo'with React Compiler♻️ Option A: Keep memo() HOC only
const HeaderContainer = memo(({ isMasterDetail = false, customRightIcon, customLeftIcon, children }: IHeaderContainer) => { - 'use memo'; - const insets = useSafeAreaInsets();♻️ Option B: Use directive only (requires React Compiler)
-const HeaderContainer = memo(({ isMasterDetail = false, customRightIcon, customLeftIcon, children }: IHeaderContainer) => { +const HeaderContainer = ({ isMasterDetail = false, customRightIcon, customLeftIcon, children }: IHeaderContainer) => { 'use memo'; const insets = useSafeAreaInsets(); // ... -}); +};android/app/src/main/AndroidManifest.xml (2)
24-25: Consider addingandroid:required="false"for broader device compatibility.Without
android:required="false", devices lacking these hardware features will be filtered out from the Play Store listing. Unless VoIP is absolutely essential and the app cannot function without audio/microphone hardware, consider:- <uses-feature android:name="android.hardware.audio.output" /> - <uses-feature android:name="android.hardware.microphone" /> + <uses-feature android:name="android.hardware.audio.output" android:required="false" /> + <uses-feature android:name="android.hardware.microphone" android:required="false" />
119-128: Update the service label from "Wazo" to the app name.The
android:label="Wazo"appears to be a default from the react-native-callkeep library. This label may be visible to users in system settings or call management UI. Consider using the app's string resource:<service android:name="io.wazo.callkeep.VoiceConnectionService" - android:label="Wazo" + android:label="@string/app_name" android:permission="android.permission.BIND_TELECOM_CONNECTION_SERVICE"app/lib/services/voip/useCallStore.ts (2)
126-140: Optimistic state updates may cause UI/call state desync.
toggleMuteandtoggleHoldupdate local state immediately without confirming the call operation succeeded. Ifcall.setMuted()orcall.setHeld()fails or throws, the UI will show incorrect state.Consider relying on the
trackStateChangeevent handler to update state, or add error handling:toggleMute: () => { const { call, isMuted } = get(); if (!call) return; - call.setMuted(!isMuted); - set({ isMuted: !isMuted }); + try { + call.setMuted(!isMuted); + // State will be updated by trackStateChange handler + } catch (error) { + console.error('Failed to toggle mute:', error); + } },
67-67: Remove incomplete comment.This appears to be leftover from development. Either complete the implementation or remove the comment.
- // isSpeakerOn: call.app/containers/CallHeader/components/Collapse.tsx (1)
10-11: Consider combining store selectors to reduce subscriptions.Two separate
useCallStorecalls create two subscriptions. While the impact is minimal, combining them withuseShallowis more efficient:-const focused = useCallStore(state => state.focused); -const toggleFocus = useCallStore(state => state.toggleFocus); +const { focused, toggleFocus } = useCallStore( + useShallow(state => ({ focused: state.focused, toggleFocus: state.toggleFocus })) +);Note: For the action selector (
toggleFocus), this is a stable reference so separate selection is acceptable. This is a minor optimization.app/views/CallView/components/CallStatusText.tsx (1)
15-23: Unnecessary fragment wrapper.The fragment
<>...</>wrapping the singleTextelement is superfluous. The outerTextelement can be returned directly.Suggested simplification
if (isOnHold && isMuted) { return ( - <> - <Text style={[styles.statusText, { color: colors.fontDefault }]}> - {I18n.t('On_hold')}, <Text style={{ color: colors.statusFontWarning }}>{I18n.t('Muted')}</Text> - </Text> - </> + <Text style={[styles.statusText, { color: colors.fontDefault }]}> + {I18n.t('On_hold')}, <Text style={{ color: colors.statusFontWarning }}>{I18n.t('Muted')}</Text> + </Text> ); }app/lib/services/voip/simulateCall.ts (1)
96-102: Redundant state update.The
setStatecall here is unnecessary. WhensetCallis invoked on line 94, it already readsisMutedandisOnHoldfrom the mock's getters (call.mutedandcall.held), which return the correct initial values.Suggested cleanup
// Set up the call store useCallStore.getState().setCall(mockCall, callUUID); - - // If simulating a specific initial state, update the store - if (isMuted || isOnHold) { - useCallStore.setState({ - isMuted, - isOnHold - }); - }app/views/SidebarView/components/Stacks.tsx (1)
36-36: Incomplete implementation: commented-outbackgroundColor.The
backgroundColorprop is commented out. Consider either implementing it properly (tracking current screen for Voice_call) or removing the comment if it's not needed.app/views/CallView/index.tsx (2)
19-29: Consider consolidating store selectors to reduce subscription overhead.Each
useCallStore(state => state.X)call creates a separate subscription. While Zustand handles this efficiently, consolidating related selectors can improve performance and reduce re-renders when unrelated state changes.♻️ Optional consolidation example
- // Get state from store - const call = useCallStore(state => state.call); - const callState = useCallStore(state => state.callState); - const isMuted = useCallStore(state => state.isMuted); - const isOnHold = useCallStore(state => state.isOnHold); - const isSpeakerOn = useCallStore(state => state.isSpeakerOn); - - // Get actions from store - const toggleMute = useCallStore(state => state.toggleMute); - const toggleHold = useCallStore(state => state.toggleHold); - const toggleSpeaker = useCallStore(state => state.toggleSpeaker); - const endCall = useCallStore(state => state.endCall); + // Get state and actions from store + const { call, callState, isMuted, isOnHold, isSpeakerOn, toggleMute, toggleHold, toggleSpeaker, endCall } = useCallStore( + state => ({ + call: state.call, + callState: state.callState, + isMuted: state.isMuted, + isOnHold: state.isOnHold, + isSpeakerOn: state.isSpeakerOn, + toggleMute: state.toggleMute, + toggleHold: state.toggleHold, + toggleSpeaker: state.toggleSpeaker, + endCall: state.endCall + }) + );Note: If you consolidate, consider using Zustand's
shallowequality check to avoid unnecessary re-renders:import { shallow } from 'zustand/shallow'; const { ... } = useCallStore(state => ({ ... }), shallow);
50-52: ThehandleEndCallwrapper can be removed.This wrapper simply calls
endCall()without any additional logic. You can passendCalldirectly toonPress.♻️ Simplify by removing wrapper
- const handleEndCall = () => { - endCall(); - };Then on line 104:
- onPress={handleEndCall} + onPress={endCall}app/views/CallView/index.test.tsx (1)
312-336: Icon state tests don't verify actual icon values.These tests only check that the button exists, not that the correct icon is displayed. The comments acknowledge this ("we test behavior rather than props"). Consider verifying the actual icon via accessibility props or snapshot testing if icon correctness is important.
app/views/CallView/components/CallerInfo.test.tsx (1)
56-68: Test name references internal implementation detail.The test name "should render status container (Status component is currently commented out)" describes internal code state rather than expected behavior. Consider renaming to focus on the observable behavior being tested.
♻️ Suggested rename
- it('should render status container (Status component is currently commented out)', () => { + it('should render caller info with avatar', () => {app/definitions/Voip.ts (1)
1-5: Consider supporting array type forurlsproperty to align with WebRTC standards.The standard WebRTC
RTCIceServerinterface allowsurlsto be eitherstringorstring[]to specify multiple URLs for a single ICE server. While the current implementation only uses single URLs, supporting both types would improve flexibility:export type IceServer = { - urls: string; + urls: string | string[]; username?: string; credential?: string; };app/lib/services/voip/MediaSessionStore.ts (2)
15-16: Consider using crypto-secure random string generation.The current implementation uses
Math.random()which is not cryptographically secure. If these IDs are used in security-sensitive contexts (like session identification), consider using@rocket.chat/mobile-crypto'srandomUuidwhich is already imported elsewhere in this PR.
97-98: Address the TODO comment before merging.The TODO indicates the singleton name should be changed. Consider finalizing the naming now to avoid technical debt.
Would you like me to suggest alternative names based on the class's responsibility, or open an issue to track this?
app/views/CallView/CallView.stories.tsx (2)
19-28: Remove or explain commented-out code.This commented jest mock is dead code in a Storybook file. If it's intended as documentation for future test setup, add a comment explaining why it's kept. Otherwise, remove it.
32-49: Consider adding proper type for mockCall.The
as anytype assertion bypasses TypeScript safety. Consider creating a proper mock type or partial type to maintain type checking while allowing incomplete mock objects.Suggested typing approach
import type { IClientMediaCall } from '@rocket.chat/media-signaling'; const mockCall: Partial<IClientMediaCall> & { emitter: { on: () => void; off: () => void } } = { // ... properties };app/lib/services/voip/MediaSessionInstance.ts (2)
88-101: Consider error handling for async accept() call.The
answerCallhandler awaitsmainCall.accept()but doesn't handle potential rejection. If accept fails, the UI state may become inconsistent.Suggested error handling
this.callKeepListeners.answerCall = RNCallKeep.addEventListener('answerCall', async ({ callUUID }) => { const callId = localCallIdMap[callUUID]; const mainCall = this.instance?.getMainCall(); if (mainCall && mainCall.callId === callId) { - await mainCall.accept(); - RNCallKeep.setCurrentCallActive(callUUID); - useCallStore.getState().setCall(mainCall, callUUID); - Navigation.navigate('CallView', { callUUID }); + try { + await mainCall.accept(); + RNCallKeep.setCurrentCallActive(callUUID); + useCallStore.getState().setCall(mainCall, callUUID); + Navigation.navigate('CallView', { callUUID }); + } catch (error) { + console.error('Failed to accept call:', error); + RNCallKeep.endCall(callUUID); + useCallStore.getState().reset(); + } } else { RNCallKeep.endCall(callUUID); } });
21-21: Module-level mutable state may cause issues across hot reloads.
localCallIdMapis a module-level mutable object. During development with hot reload, this state may persist unexpectedly. Consider moving it into the class instance or adding cleanup logic.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (7)
android/app/src/main/assets/fonts/custom.ttfis excluded by!**/*.ttfapp/views/CallView/__snapshots__/index.test.tsx.snapis excluded by!**/*.snapapp/views/CallView/components/__snapshots__/CallActionButton.test.tsx.snapis excluded by!**/*.snapapp/views/CallView/components/__snapshots__/CallerInfo.test.tsx.snapis excluded by!**/*.snapios/Podfile.lockis excluded by!**/*.lockios/custom.ttfis excluded by!**/*.ttfyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (42)
__mocks__/react-native-callkeep.jsandroid/app/src/main/AndroidManifest.xmlapp/AppContainer.tsxapp/containers/CallHeader/CallHeader.tsxapp/containers/CallHeader/components/Collapse.tsxapp/containers/CallHeader/components/EndCall.tsxapp/containers/CallHeader/components/Timer.tsxapp/containers/CallHeader/components/Title.tsxapp/containers/CustomIcon/mappedIcons.jsapp/containers/CustomIcon/selection.jsonapp/containers/Header/components/HeaderContainer/index.tsxapp/definitions/Voip.tsapp/i18n/locales/en.jsonapp/lib/constants/defaultSettings.tsapp/lib/services/voip/MediaCallLogger.tsapp/lib/services/voip/MediaSessionInstance.tsapp/lib/services/voip/MediaSessionStore.tsapp/lib/services/voip/parseStringToIceServers.tsapp/lib/services/voip/simulateCall.tsapp/lib/services/voip/useCallStore.tsapp/lib/store/index.tsapp/sagas/login.jsapp/stacks/InsideStack.tsxapp/stacks/types.tsapp/views/CallView/CallView.stories.tsxapp/views/CallView/components/CallActionButton.stories.tsxapp/views/CallView/components/CallActionButton.test.tsxapp/views/CallView/components/CallActionButton.tsxapp/views/CallView/components/CallStatusText.tsxapp/views/CallView/components/CallerInfo.stories.tsxapp/views/CallView/components/CallerInfo.test.tsxapp/views/CallView/components/CallerInfo.tsxapp/views/CallView/index.test.tsxapp/views/CallView/index.tsxapp/views/CallView/styles.tsapp/views/RoomView/components/HeaderCallButton.tsxapp/views/SidebarView/components/Stacks.tsxapp/views/SidebarView/index.tsxios/RocketChatRN.xcodeproj/project.pbxprojpackage.jsonpatches/@rocket.chat+sdk+1.3.3-mobile.patchpatches/react-native-callkeep+4.3.16.patch
🧰 Additional context used
🧬 Code graph analysis (16)
app/views/CallView/components/CallerInfo.tsx (6)
app/theme.tsx (1)
useTheme(29-29)app/lib/services/voip/useCallStore.ts (1)
useCallContact(184-184)app/views/CallView/styles.ts (1)
styles(5-74)app/containers/message/MessageAvatar.tsx (1)
AvatarContainer(13-17)app/lib/constants/colors.ts (1)
colors(280-302)app/containers/CustomIcon/index.tsx (1)
CustomIcon(47-47)
app/views/CallView/components/CallerInfo.test.tsx (4)
app/lib/services/voip/useCallStore.ts (1)
useCallStore(56-180)jest.setup.js (1)
React(179-179)app/reducers/mockedStore.ts (1)
mockedStore(7-7).rnstorybook/generateSnapshots.tsx (1)
generateSnapshots(10-22)
app/containers/CallHeader/components/EndCall.tsx (3)
app/theme.tsx (1)
useTheme(29-29)app/lib/services/voip/useCallStore.ts (1)
useCallStore(56-180)app/lib/constants/colors.ts (1)
colors(280-302)
app/containers/CallHeader/CallHeader.tsx (3)
app/theme.tsx (1)
useTheme(29-29)app/lib/constants/colors.ts (1)
colors(280-302)app/lib/services/voip/useCallStore.ts (1)
useCallStore(56-180)
app/lib/services/voip/simulateCall.ts (2)
app/lib/methods/helpers/emitter.ts (1)
emitter(24-24)app/lib/services/voip/useCallStore.ts (1)
useCallStore(56-180)
app/sagas/login.js (2)
app/lib/services/voip/MediaSessionInstance.ts (1)
mediaSessionInstance(187-187)app/lib/services/voip/simulateCall.ts (1)
simulateCall(28-106)
app/views/CallView/index.tsx (4)
app/theme.tsx (1)
useTheme(29-29)app/lib/services/voip/useCallStore.ts (1)
useCallStore(56-180)app/views/CallView/styles.ts (1)
styles(5-74)app/lib/constants/colors.ts (1)
colors(280-302)
app/lib/services/voip/parseStringToIceServers.ts (1)
app/definitions/Voip.ts (1)
IceServer(1-5)
app/views/CallView/components/CallActionButton.test.tsx (1)
.rnstorybook/generateSnapshots.tsx (1)
generateSnapshots(10-22)
app/containers/CallHeader/components/Title.tsx (3)
app/theme.tsx (1)
useTheme(29-29)app/lib/services/voip/useCallStore.ts (1)
useCallStore(56-180)app/lib/constants/colors.ts (1)
colors(280-302)
app/views/CallView/index.test.tsx (3)
app/lib/services/voip/useCallStore.ts (1)
useCallStore(56-180)app/reducers/mockedStore.ts (1)
mockedStore(7-7).rnstorybook/generateSnapshots.tsx (1)
generateSnapshots(10-22)
app/views/SidebarView/components/Stacks.tsx (2)
app/lib/services/voip/useCallStore.ts (1)
useCallStore(56-180)app/lib/services/voip/simulateCall.ts (1)
simulateCall(28-106)
app/containers/CallHeader/components/Timer.tsx (1)
app/lib/services/voip/useCallStore.ts (1)
useCallStore(56-180)
app/AppContainer.tsx (5)
app/containers/CallHeader.tsx (1)
CallHeader(28-83)app/lib/methods/helpers/emitter.ts (1)
emitter(24-24)app/lib/methods/helpers/navigation/index.ts (1)
getActiveRouteName(50-50)app/lib/methods/helpers/log/index.ts (1)
setCurrentScreen(44-47)app/lib/constants/colors.ts (1)
themes(304-304)
app/views/CallView/components/CallStatusText.tsx (3)
app/lib/services/voip/useCallStore.ts (1)
useCallControls(185-192)app/theme.tsx (1)
useTheme(29-29)app/views/CallView/styles.ts (1)
styles(5-74)
app/views/RoomView/components/HeaderCallButton.tsx (1)
app/lib/services/voip/useCallStore.ts (1)
useCallStore(56-180)
🪛 GitHub Actions: Format Code with Prettier
package.json
[error] yarn install --frozen-lockfile failed. Package "" refers to a non-existing file '/Users/diegomello/Development/Work/Rocket.Chat/packages/media-signaling'.
🔇 Additional comments (34)
ios/RocketChatRN.xcodeproj/project.pbxproj (1)
1999-2037: CocoaPods configuration updates for VOIP support look correct.The addition of
JitsiWebRTC/WebRTC.frameworkto the Embed Pods Frameworks phase for bothRocketChatRNandRocket.Chattargets is properly configured for the voice support feature. The framework embedding is correctly set up with:
- Input path:
${PODS_XCFRAMEWORKS_BUILD_DIR}/JitsiWebRTC/WebRTC.framework/WebRTC- Output path:
${TARGET_BUILD_DIR}/${FRAMEWORKS_FOLDER_PATH}/WebRTC.frameworkThe rest of the changes are standard CocoaPods-generated updates with consistent naming convention (
Pods-defaults-*) across all targets and build configurations.patches/@rocket.chat+sdk+1.3.3-mobile.patch (1)
1-14: LGTM — new DDP subscriptions for VoIP events are properly integrated.The patch correctly adds
media-signalandmedia-callsevent subscriptions alongside the existingvideo-conferencetopic. These subscriptions are actively used by the VoIP service implementations inapp/lib/services/voip/to handle real-time call signaling events per user.A few notes:
- Patch maintenance: When upgrading
@rocket.chat/sdk, ensure this patch still applies cleanly or upstream the changes if possible.- Server compatibility: Confirm the Rocket.Chat server version in use supports these subscription topics for streaming user-specific media events.
app/containers/CustomIcon/mappedIcons.js (1)
160-160: LGTM!The new
pause-shape-unfilledicon mapping follows the existing pattern and uses a unique icon code. This aligns with the VoIP call UI requirements.app/stacks/InsideStack.tsx (2)
69-69: LGTM!Import follows the established pattern for view components.
343-343: LGTM!The
CallViewscreen registration follows the same pattern asJitsiMeetViewwithheaderShown: false, which is appropriate for a full-screen call interface. The route is correctly typed inInsideStackParamList.app/stacks/types.ts (1)
299-301: LGTM!The
CallViewroute parameter type is correctly defined withcallUUID: string, providing type-safe navigation to the call screen.app/lib/constants/defaultSettings.ts (1)
309-314: LGTM!The new VoIP ICE server settings follow the established pattern. The types are appropriate:
valueAsStringfor ICE servers configuration (parsed byparseStringToIceServers)valueAsNumberfor the gathering timeoutpackage.json (2)
144-144: LGTM for Zustand addition.Zustand
^5.0.10is a solid choice for call state management. It's lightweight and works well with React 19.
125-125: Testreact-native-webrtcwith React Native 0.79.4 before merge.The package version 124.0.7 is compatible with React Native 0.79.4, but React Native 0.79 introduces breaking changes to package exports and module syntax that can affect native-module packages. Ensure your project's imports and any deep imports from this package work correctly after upgrading.
app/i18n/locales/en.json (1)
325-325: LGTM!New translation keys for VoIP features are correctly added in alphabetical order and follow existing naming conventions.
app/views/SidebarView/index.tsx (2)
15-15: Good use of React Compiler directive.The
'use memo'directive is the correct React 19 Compiler approach, replacing the need for manualReact.memo()wrapping.
27-34: Verify ScrollView in drawer respects safe area on notched devices.SafeAreaProvider wraps the entire app at the root (app/index.tsx), and SidebarView is used as drawer content in the navigation stack. However, the drawer navigator has no explicit safe area handling in its configuration, and the SidebarView contains only a ScrollView without an explicit SafeAreaView wrapper. While React Navigation's drawer navigator should respect safe area insets from the provider context, confirm that the sidebar content doesn't render under the status bar or notch on iOS devices, especially since other views in the codebase explicitly use the custom SafeAreaView component.
app/views/CallView/styles.ts (1)
60-67: Consider using theme colors for consistency.The
actionButtonstyle uses hardcoded dimensions which is fine, but note that the background color will need to be applied dynamically based on button state/type. Ensure the consuming component applies theme-aware colors.app/views/CallView/components/CallerInfo.tsx (1)
17-54: LGTM on component structure and conditional rendering.The fallback logic for name/avatar text and the conditional rendering of muted indicator and extension are well implemented. Good use of
testIDattributes for testability.app/containers/Header/components/HeaderContainer/index.tsx (1)
19-20: Verify notch handling with hardcodedpaddingTop: 4.The previous dynamic notch padding logic has been replaced with a fixed
paddingTop: 4. Per the PR summary, the newCallHeadercomponent handles notch padding separately.Please verify this header is never rendered as the topmost element on notched devices, or that the parent component provides adequate safe area insets.
app/views/CallView/components/CallActionButton.stories.tsx (1)
1-61: LGTM!Well-structured Storybook stories that comprehensively demonstrate all variants of the
CallActionButtoncomponent. TheAllVariantsstory provides a useful visual overview for development.app/views/CallView/components/CallActionButton.test.tsx (1)
1-60: LGTM!Good test coverage including rendering, label display, press handling, disabled state behavior, and variant rendering. The combination of unit tests with
generateSnapshotsfor visual regression testing is a solid approach.app/lib/services/voip/useCallStore.ts (1)
182-199: LGTM on selector hooks.Good use of
useShallowforuseCallControlsto prevent unnecessary re-renders when selecting multiple primitive values. The selector pattern provides good performance optimization.android/app/src/main/AndroidManifest.xml (1)
22-23: No action needed—MANAGE_OWN_CALLSis not required.
MANAGE_OWN_CALLSis only required when using react-native-callkeep'sselfManagedmode. This app uses the default managed mode (noselfManaged: truein the setup options), so the permission should remain commented out.app/views/CallView/components/CallStatusText.tsx (1)
9-13: LGTM!The component cleanly handles all status combinations and integrates well with the VoIP store via
useCallControls. The use of the'use memo'directive aligns with the React Compiler optimization pattern used elsewhere in the codebase.app/sagas/login.js (1)
296-297: Arbitrary delay before VoIP initialization.The 1-second delay appears arbitrary. If it's needed to wait for specific conditions, consider using a more deterministic approach (e.g., waiting for a specific state or event). If it's for debugging, please remove or document why it's necessary.
app/views/RoomView/components/HeaderCallButton.tsx (1)
29-38: Behavioral change: button always renders regardless of call capability.The previous implementation only rendered the button when
callEnabledwas true. Now it always renders and callstoggleFocus(). Verify this is intentional—users may see a call button in contexts where VoIP isn't actually available for the room.app/containers/CallHeader/components/EndCall.tsx (1)
6-16: LGTM!The
EndCallcomponent is well-structured with proper accessibility support, theme integration, and correct usage of the Zustand store selector pattern for theendCallaction.app/views/CallView/index.tsx (1)
61-111: LGTM!The UI composition is well-structured with proper conditional rendering, dynamic button states, and appropriate testIDs for testing. The
isConnectingandisConnectedderivations correctly handle the call state transitions.app/views/CallView/index.test.tsx (1)
81-112: LGTM!Good test coverage for component presence and action button rendering. The test structure with
beforeEachreset and helper functions is clean and maintainable.app/views/CallView/components/CallerInfo.stories.tsx (1)
31-44: LGTM!The Storybook meta configuration with decorator for store initialization is well-structured and provides a clean pattern for stories that need pre-populated state.
app/views/CallView/components/CallerInfo.test.tsx (1)
27-102: LGTM!The test suite provides good coverage for the
CallerInfocomponent, including edge cases for missing display name, muted indicator visibility, and extension presence. The use ofbeforeEachfor store reset ensures test isolation.app/AppContainer.tsx (1)
55-87: LGTM on the component structure.The
CallHeaderplacement aboveNavigationContaineris appropriate for a persistent VoIP header that should remain visible during navigation transitions.app/containers/CallHeader/components/Timer.tsx (1)
14-33: LGTM on the Timer implementation.The timer logic and interval cleanup are correctly implemented. The nested
Textcomponent will inherit styles from the parentTextinTitle.tsx.app/lib/services/voip/parseStringToIceServers.ts (1)
3-16: LGTM on the parsing logic.The credential extraction and URL decoding logic correctly handles the
username:credential@urlformat, and appropriately omits credentials when not fully present.app/containers/CallHeader/CallHeader.tsx (1)
21-44: LGTM! Clean conditional rendering for call header.The component correctly uses the
'use memo'directive (React 19 compiler feature) and handles the no-call state appropriately with an early return. The separation of concerns withCollapse,Title, andEndCallsubcomponents is well-structured.One minor observation:
defaultHeaderStyleis recreated on each render. If performance becomes a concern, consider memoizing it withuseMemosince it depends oncolors.surfaceNeutralandinsets.top.app/lib/services/voip/MediaSessionStore.ts (2)
45-68: Good session lifecycle management.The
makeInstancemethod properly ends any existing session before creating a new one, preventing resource leaks. The validation of required dependencies before session creation is also a good safeguard.
70-80: The current validation withif (!userId)already handles empty strings correctly. Since empty string is falsy in JavaScript,!''evaluates totrue, which means the error will be thrown for empty strings just as intended. No changes are needed.app/lib/services/voip/MediaSessionInstance.ts (1)
166-184: Good cleanup implementation in stop().The
stop()method properly cleans up all listeners, subscriptions, and the session instance. The iteration overlocalCallIdMapkeys to delete entries ensures no stale mappings remain.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| return ( | ||
| <Text style={[styles.headerTitle, { color: colors.fontDefault }]} testID='call-view-header-title'> | ||
| {getHeaderTitle()} | ||
| <Timer /> | ||
| </Text> | ||
| ); |
There was a problem hiding this comment.
Timer renders unconditionally, even during "Connecting" state.
The Timer component is always rendered, but the title logic at line 31 returns "Connecting" without the timer. This results in displaying "Connecting00:00" when isConnecting is true.
return (
<Text style={[styles.headerTitle, { color: colors.fontDefault }]} testID='call-view-header-title'>
{getHeaderTitle()}
- <Timer />
+ {isConnected && callStartTime ? <Timer /> : null}
</Text>
);🤖 Prompt for AI Agents
In `@app/containers/CallHeader/components/Title.tsx` around lines 39 - 44, The
Timer is always rendered causing "Connecting00:00"; update the JSX in Title.tsx
to render <Timer /> only when the header is not in the connecting state—use the
existing isConnecting flag (or the result of getHeaderTitle() === "Connecting")
to conditionally include Timer so when isConnecting is true the Timer is
omitted; locate the getHeaderTitle() call and the Timer component in the return
block and wrap Timer with a conditional check tied to isConnecting (or title
check).
| this.storeIceServersUnsubscribe = store.subscribe(() => { | ||
| const currentIceServers = this.getIceServers(); | ||
| if (currentIceServers !== this.iceServers) { | ||
| this.iceServers = currentIceServers; | ||
| // this.instance?.setIceServers(this.iceServers); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Array reference comparison will always trigger update; commented code indicates incomplete feature.
Line 159 compares array references (!==), not contents. Since getIceServers() returns a new array instance each time, this condition will always be true, causing unnecessary updates. Additionally, the commented-out setIceServers call suggests this feature is incomplete.
Suggested fix with deep comparison
this.storeIceServersUnsubscribe = store.subscribe(() => {
const currentIceServers = this.getIceServers();
- if (currentIceServers !== this.iceServers) {
+ if (JSON.stringify(currentIceServers) !== JSON.stringify(this.iceServers)) {
this.iceServers = currentIceServers;
- // this.instance?.setIceServers(this.iceServers);
+ // TODO: Implement setIceServers on MediaSignalingSession
+ // this.instance?.setIceServers(this.iceServers);
}
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.storeIceServersUnsubscribe = store.subscribe(() => { | |
| const currentIceServers = this.getIceServers(); | |
| if (currentIceServers !== this.iceServers) { | |
| this.iceServers = currentIceServers; | |
| // this.instance?.setIceServers(this.iceServers); | |
| } | |
| }); | |
| this.storeIceServersUnsubscribe = store.subscribe(() => { | |
| const currentIceServers = this.getIceServers(); | |
| if (JSON.stringify(currentIceServers) !== JSON.stringify(this.iceServers)) { | |
| this.iceServers = currentIceServers; | |
| // TODO: Implement setIceServers on MediaSignalingSession | |
| // this.instance?.setIceServers(this.iceServers); | |
| } | |
| }); |
🤖 Prompt for AI Agents
In `@app/lib/services/voip/MediaSessionInstance.ts` around lines 157 - 163, The
subscription currently compares array references (getIceServers() !==
this.iceServers) which always differs because getIceServers returns a new array;
replace this with a deep/element-wise equality check (e.g., use a utility like
lodash/isEqual or compare lengths and each entry) to detect real changes, only
assign this.iceServers and call the previously commented
this.instance?.setIceServers(this.iceServers) when the contents actually differ,
and ensure this.instance exists (guard or optional chaining) before calling
setIceServers; alternatively, modify getIceServers to return a stable/memoized
reference to avoid churn.
| setCall: (call: IClientMediaCall, callUUID: string) => { | ||
| // Update state with call info | ||
| set({ | ||
| call, | ||
| callUUID, | ||
| callState: call.state, | ||
| isMuted: call.muted, | ||
| isOnHold: call.held, | ||
| // isSpeakerOn: call. | ||
| contact: { | ||
| displayName: call.contact.displayName, | ||
| username: call.contact.username, | ||
| sipExtension: call.contact.sipExtension | ||
| }, | ||
| callStartTime: call.state === 'active' ? Date.now() : null | ||
| }); | ||
|
|
||
| // Subscribe to call events | ||
| const handleStateChange = () => { | ||
| const currentCall = get().call; | ||
| if (!currentCall) return; | ||
|
|
||
| const newState = currentCall.state; | ||
| set({ callState: newState }); | ||
|
|
||
| // Set start time when call becomes active | ||
| if (newState === 'active' && !get().callStartTime) { | ||
| set({ callStartTime: Date.now() }); | ||
| } | ||
| }; | ||
|
|
||
| const handleTrackStateChange = () => { | ||
| const currentCall = get().call; | ||
| if (!currentCall) return; | ||
|
|
||
| set({ | ||
| isMuted: currentCall.muted, | ||
| isOnHold: currentCall.held | ||
| }); | ||
| }; | ||
|
|
||
| const handleEnded = () => { | ||
| get().reset(); | ||
| Navigation.back(); | ||
| }; | ||
|
|
||
| call.emitter.on('stateChange', handleStateChange); | ||
| call.emitter.on('trackStateChange', handleTrackStateChange); | ||
| call.emitter.on('ended', handleEnded); | ||
| }, |
There was a problem hiding this comment.
Memory leak: Event listeners are never removed.
The setCall function subscribes to call events (stateChange, trackStateChange, ended) but never stores or removes these listeners. This causes:
- Memory leaks when calls end
- Multiple callbacks firing if
setCallis called again - Callbacks referencing stale state via closure
🐛 Proposed fix to track and clean up listeners
interface CallStoreState {
// Call reference
call: IClientMediaCall | null;
callUUID: string | null;
+ callCleanup: (() => void) | null;
// ... rest of state
}
const initialState: CallStoreState = {
call: null,
callUUID: null,
+ callCleanup: null,
// ... rest of initial state
};
setCall: (call: IClientMediaCall, callUUID: string) => {
+ // Clean up previous call listeners
+ const previousCleanup = get().callCleanup;
+ if (previousCleanup) {
+ previousCleanup();
+ }
// ... existing set() call ...
const handleStateChange = () => { /* ... */ };
const handleTrackStateChange = () => { /* ... */ };
const handleEnded = () => {
+ cleanup();
get().reset();
Navigation.back();
};
call.emitter.on('stateChange', handleStateChange);
call.emitter.on('trackStateChange', handleTrackStateChange);
call.emitter.on('ended', handleEnded);
+ const cleanup = () => {
+ call.emitter.off('stateChange', handleStateChange);
+ call.emitter.off('trackStateChange', handleTrackStateChange);
+ call.emitter.off('ended', handleEnded);
+ };
+
+ set({ callCleanup: cleanup });
},
reset: () => {
+ const cleanup = get().callCleanup;
+ if (cleanup) {
+ cleanup();
+ }
set(initialState);
}🤖 Prompt for AI Agents
In `@app/lib/services/voip/useCallStore.ts` around lines 59 - 108, The setCall
function attaches listeners but never removes them; modify setCall to store the
listener functions (e.g., handleStateChange, handleTrackStateChange,
handleEnded) in the store state or a local cleanup map keyed by callUUID and
call.emitter, remove any existing listeners before attaching new ones, and
ensure you call emitter.off/removeListener for each listener when the call ends
(inside handleEnded) and when replacing/clearing the call (e.g., in reset).
Reference setCall, handleStateChange, handleTrackStateChange, handleEnded,
call.emitter.on and use call.emitter.off/removeListener to unsubscribe so
listeners are cleaned up and stale closures are avoided.
| <List.Item | ||
| title={'Voice_call'} | ||
| left={() => <List.Icon name='phone' />} | ||
| onPress={() => { | ||
| simulateCall(); | ||
| toggleFocus(); | ||
| }} | ||
| // backgroundColor={currentScreen === 'ChatsStackNavigator' ? colors.strokeLight : undefined} | ||
| testID='sidebar-voice-call' | ||
| /> |
There was a problem hiding this comment.
Race condition: simulateCall() not awaited before toggleFocus().
simulateCall() is async and sets the callUUID in the store. Calling toggleFocus() immediately after without awaiting may navigate to CallView before the call store is properly initialized, potentially with an undefined callUUID.
Proposed fix: await simulateCall before toggling focus
<List.Item
title={'Voice_call'}
left={() => <List.Icon name='phone' />}
- onPress={() => {
- simulateCall();
+ onPress={async () => {
+ await simulateCall();
toggleFocus();
}}
- // backgroundColor={currentScreen === 'ChatsStackNavigator' ? colors.strokeLight : undefined}
testID='sidebar-voice-call'
/>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <List.Item | |
| title={'Voice_call'} | |
| left={() => <List.Icon name='phone' />} | |
| onPress={() => { | |
| simulateCall(); | |
| toggleFocus(); | |
| }} | |
| // backgroundColor={currentScreen === 'ChatsStackNavigator' ? colors.strokeLight : undefined} | |
| testID='sidebar-voice-call' | |
| /> | |
| <List.Item | |
| title={'Voice_call'} | |
| left={() => <List.Icon name='phone' />} | |
| onPress={async () => { | |
| await simulateCall(); | |
| toggleFocus(); | |
| }} | |
| testID='sidebar-voice-call' | |
| /> |
🤖 Prompt for AI Agents
In `@app/views/SidebarView/components/Stacks.tsx` around lines 29 - 38, The
onPress handler for the List.Item calls simulateCall() (async) and then
toggleFocus() synchronously, causing a race where callUUID may not be set;
change the handler to be async, await simulateCall() before calling
toggleFocus(), and handle/rethrow errors as appropriate so toggleFocus() only
runs after simulateCall (refer to the onPress callback in the List.Item, the
simulateCall function, and the toggleFocus function).
package.json
Outdated
| "@react-navigation/elements": "^2.6.1", | ||
| "@react-navigation/native": "^7.1.16", | ||
| "@react-navigation/native-stack": "^7.3.23", | ||
| "@rocket.chat/media-signaling": "/Users/diegomello/Development/Work/Rocket.Chat/packages/media-signaling", |
There was a problem hiding this comment.
Critical: Local filesystem path breaks CI and other developers' builds.
The @rocket.chat/media-signaling dependency references a local path (/Users/diegomello/Development/...) that only exists on your machine. This causes the pipeline failure shown in the logs and will prevent anyone else from installing dependencies.
Replace with a proper package reference (npm registry, GitHub URL, or git+ssh).
#!/bin/bash
# Check if media-signaling is published on npm or available as a GitHub package
npm view `@rocket.chat/media-signaling` versions 2>/dev/null || echo "Not found on npm"
# Search for media-signaling in RocketChat GitHub repos
gh search repos "media-signaling" --owner=RocketChat --json name,url🧰 Tools
🪛 GitHub Actions: Format Code with Prettier
[error] yarn install --frozen-lockfile failed. Package "" refers to a non-existing file '/Users/diegomello/Development/Work/Rocket.Chat/packages/media-signaling'.
🤖 Prompt for AI Agents
In `@package.json` at line 50, The package.json currently points the dependency
"@rocket.chat/media-signaling" to a local filesystem path that breaks CI and
other developers' installs; update the dependency entry for
"@rocket.chat/media-signaling" in package.json to a proper registry or VCS
reference (for example an npm package name with a version, a GitHub URL like
"github:RocketChat/media-signaling#tag", or a git+ssh URL), replacing the
absolute local path and ensure the version/ref used is published or reachable so
installs succeed for everyone.
package.json
Outdated
| "react-native-animatable": "1.3.3", | ||
| "react-native-background-timer": "2.4.1", | ||
| "react-native-bootsplash": "^6.3.8", | ||
| "react-native-callkeep": "^4.3.16", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
react-native-callkeep 4.3.16 compatibility React Native 0.79
💡 Result:
Short answer: react-native-callkeep 4.3.16 can be used with React Native 0.79, but there are reported compatibility issues — several GitHub issues show runtime/behaviour problems on Android and iOS with RN 0.79 (and with the New Architecture). Test carefully and watch the repo issues for fixes. [1][2][3]
Sources:
- GitHub issue reporting Android behavior with RN 0.79 / callkeep 4.3.16. [1]
- GitHub issue reporting iOS "No such module / New Architecture" problems with callkeep 4.3.16. [2]
- npm / Expo config-plugin listing that ties react-native-callkeep 4.3.16 to recent Expo / RN setups (shows 4.3.16 is the published version in use). [3]
Would you like links to the exact GitHub issues or help troubleshooting a specific error?
Address known react-native-callkeep@4.3.16 compatibility issues with React Native 0.79.
This version has reported runtime and behavior problems on both Android and iOS with React Native 0.79, including Android behavior issues and iOS module problems with the New Architecture. Review the related GitHub issues in the react-native-callkeep repository and test thoroughly before merging, or consider using a newer compatible version if available.
🧰 Tools
🪛 GitHub Actions: Format Code with Prettier
[error] yarn install --frozen-lockfile failed. Package "" refers to a non-existing file '/Users/diegomello/Development/Work/Rocket.Chat/packages/media-signaling'.
🤖 Prompt for AI Agents
In `@package.json` at line 95, The package.json currently pins
react-native-callkeep to "react-native-callkeep": "^4.3.16", which is known to
have runtime compatibility issues with React Native 0.79; update the dependency
to a version proven compatible (or remove/replace it) and run full Android/iOS
smoke tests (including New Architecture on iOS) before merging — locate the
dependency entry "react-native-callkeep" in package.json, check upstream
issues/PRs for a recommended version or patch, change the version spec
accordingly (or replace with an alternate package), then run yarn/npm install
and platform-specific tests to verify fixes.
| +INJECTED from /Users/diegomello/Development/Work/Rocket.Chat.ReactNative/node_modules/react-native-callkeep/android/src/main/AndroidManifest.xml | ||
| +INJECTED from /Users/diegomello/Development/Work/Rocket.Chat.ReactNative/node_modules/react-native-callkeep/android/src/main/AndroidManifest.xml | ||
| + android:targetSdkVersion | ||
| + INJECTED from /Users/diegomello/Development/Work/Rocket.Chat.ReactNative/node_modules/react-native-callkeep/android/src/main/AndroidManifest.xml | ||
| + android:minSdkVersion | ||
| + INJECTED from /Users/diegomello/Development/Work/Rocket.Chat.ReactNative/node_modules/react-native-callkeep/android/src/main/AndroidManifest.xml | ||
| diff --git a/node_modules/react-native-callkeep/android/build/tmp/compileDebugJavaWithJavac/previous-compilation-data.bin b/node_modules/react-native-callkeep/android/build/tmp/compileDebugJavaWithJavac/previous-compilation-data.bin | ||
| new file mode 100644 | ||
| index 0000000..b44a073 | ||
| Binary files /dev/null and b/node_modules/react-native-callkeep/android/build/tmp/compileDebugJavaWithJavac/previous-compilation-data.bin differ | ||
| diff --git a/node_modules/react-native-callkeep/android/src/main/java/io/wazo/callkeep/RNCallKeepModule.java b/node_modules/react-native-callkeep/android/src/main/java/io/wazo/callkeep/RNCallKeepModule.java | ||
| index 025480a..70a1305 100644 | ||
| --- a/node_modules/react-native-callkeep/android/src/main/java/io/wazo/callkeep/RNCallKeepModule.java | ||
| +++ b/node_modules/react-native-callkeep/android/src/main/java/io/wazo/callkeep/RNCallKeepModule.java | ||
| @@ -434,11 +434,6 @@ public class RNCallKeepModule extends ReactContextBaseJavaModule implements Life | ||
| this.hasListeners = false; | ||
| } | ||
|
|
||
| - @ReactMethod | ||
| - public void displayIncomingCall(String uuid, String number, String callerName) { | ||
| - this.displayIncomingCall(uuid, number, callerName, false, null); | ||
| - } | ||
| - | ||
| @ReactMethod | ||
| public void displayIncomingCall(String uuid, String number, String callerName, boolean hasVideo) { | ||
| this.displayIncomingCall(uuid, number, callerName, hasVideo, null); | ||
| @@ -483,11 +478,6 @@ public class RNCallKeepModule extends ReactContextBaseJavaModule implements Life | ||
| conn.onAnswer(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for calls that might be using the old 3-parameter signatures
echo "=== Searching for displayIncomingCall calls ==="
rg -n "displayIncomingCall\s*\(" --type ts --type tsx --type js -C 2
echo ""
echo "=== Searching for startCall calls ==="
rg -n "startCall\s*\(" --type ts --type tsx --type js -C 2
echo ""
echo "=== Check for mock/test files ==="
fd -t f "mock\|callkeep" --type f | head -20Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 272
🏁 Script executed:
#!/bin/bash
# Search more broadly without type restrictions
echo "=== Searching for displayIncomingCall calls (no type filter) ==="
rg "displayIncomingCall" -C 2
echo ""
echo "=== Searching for startCall calls (no type filter) ==="
rg "startCall" -C 2
echo ""
echo "=== List JS/TS files in repo ==="
fd -e js -e ts -e jsx -e tsx --type f | head -30
echo ""
echo "=== Check src directory structure ==="
ls -la src/ 2>/dev/null || echo "No src directory"Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 3721
🏁 Script executed:
#!/bin/bash
echo "=== Full context of displayIncomingCall call in MediaSessionInstance.ts ==="
cat -n app/lib/services/voip/MediaSessionInstance.ts | sed -n '1,150p'
echo ""
echo "=== Search for all displayIncomingCall usages with line numbers ==="
rg -n "displayIncomingCall" app/lib/services/voip/MediaSessionInstance.ts -B 5 -A 5Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 7189
🏁 Script executed:
#!/bin/bash
echo "=== Search full patch for displayIncomingCall signatures ==="
grep -n "displayIncomingCall" patches/react-native-callkeep+4.3.16.patch | head -40
echo ""
echo "=== Full mock file content ==="
cat __mocks__/react-native-callkeep.jsRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 764
🏁 Script executed:
#!/bin/bash
echo "=== Full patch file to see complete method signatures ==="
wc -l patches/react-native-callkeep+4.3.16.patch
echo ""
echo "=== Search for all method signatures in patch (5-param version) ==="
grep -A 3 "public void displayIncomingCall" patches/react-native-callkeep+4.3.16.patch
echo ""
echo "=== Check if there are comments or docs about parameter order ==="
grep -B 5 -A 10 "displayIncomingCall.*uuid.*number.*callerName" patches/react-native-callkeep+4.3.16.patch | head -50Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 1654
🏁 Script executed:
#!/bin/bash
echo "=== Full patch content around lines 530-540 to see context ==="
sed -n '520,544p' patches/react-native-callkeep+4.3.16.patch
echo ""
echo "=== Search for 5-parameter displayIncomingCall in entire patch ==="
grep -n "uuid.*number.*callerName.*hasVideo.*null" patches/react-native-callkeep+4.3.16.patch
echo ""
echo "=== Search for startCall usage in TypeScript files ==="
rg "startCall" -C 3Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 2319
Fix displayIncomingCall invocation in MediaSessionInstance.ts — signature mismatch after method removal.
The patch removes the 3-parameter convenience overload for displayIncomingCall(), but the call at line 78 in app/lib/services/voip/MediaSessionInstance.ts is passing an incorrect signature:
RNCallKeep.displayIncomingCall(callUUID, displayName, displayName, 'generic', false);
After the patch, the method expects (uuid, number, callerName, hasVideo: boolean), but the code is passing 'generic' (string) instead of a boolean for the hasVideo parameter. Update this call to match the post-patch 4-parameter signature:
-RNCallKeep.displayIncomingCall(callUUID, displayName, displayName, 'generic', false);
+RNCallKeep.displayIncomingCall(callUUID, displayName, displayName, false);Verify that the removal of the 3-parameter overload is intentional and that no other call sites depend on the old signature.
🤖 Prompt for AI Agents
In `@patches/react-native-callkeep`+4.3.16.patch around lines 507 - 534, The call
to RNCallKeep.displayIncomingCall in MediaSessionInstance.ts uses the old
5-argument form and passes a string 'generic' where the new 4-argument signature
expects a boolean hasVideo; update the invocation in MediaSessionInstance.ts
(the call with RNCallKeep.displayIncomingCall(callUUID, displayName,
displayName, 'generic', false)) to match the new method
RNCallKeep.displayIncomingCall(uuid, number, callerName, hasVideo) by removing
the extraneous string and passing a boolean (e.g.,
RNCallKeep.displayIncomingCall(callUUID, displayName, displayName, false) or a
proper hasVideo variable), and then search for other usages of
RNCallKeep.displayIncomingCall to ensure no other call sites rely on the removed
3-arg overload.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@app/sagas/login.js`:
- Around line 269-278: The startVoipFork saga currently calls simulateCall(),
which is test/debug-only and must not run in production; update startVoipFork so
it no longer invokes simulateCall unconditionally—either remove the
simulateCall() call entirely from startVoipFork or guard it behind a development
check (e.g., if (__DEV__) { simulateCall(); }) so production builds never
execute it; keep the rest of the flow (call initCallKeep, select user id,
mediaSessionInstance.init(userId)) unchanged.
- Around line 257-258: The saga is calling RNCallKeep.setup(options) and
RNCallKeep.canMakeMultipleCalls(false) without yielding their returned Promises;
change both calls to be yielded so the saga waits for completion (e.g., use
redux-saga yield call to invoke RNCallKeep.setup with options and yield call to
invoke RNCallKeep.canMakeMultipleCalls with false) so subsequent CallKeep
operations run only after these async operations finish.
♻️ Duplicate comments (2)
app/sagas/login.js (2)
260-263: Remove debugsetIntervalbefore merging.This interval logs every second indefinitely and will run in production, polluting logs and wasting resources. It appears to be leftover debug code.
Proposed fix: remove debug code
RNCallKeep.setup(options); RNCallKeep.canMakeMultipleCalls(false); - - const start = Date.now(); - setInterval(() => { - console.log('Timer fired after', Date.now() - start, 'ms'); - }, 1000); } catch (e) { log(e); }
274-274: Async functionsimulateCall()not properly yielded in saga.
simulateCall()is an async function but is called withoutyield call(). This means the saga won't wait for it to complete and any errors won't be caught by the try/catch block.Proposed fix: use call effect for async function
- simulateCall(); + yield call(simulateCall);
🧹 Nitpick comments (1)
app/sagas/login.js (1)
296-297: Arbitrary 1-second delay before VOIP initialization needs justification.The hardcoded
delay(1000)before starting VOIP lacks documentation explaining why it's necessary. If it's a workaround for a race condition, consider:
- Adding a comment explaining the purpose
- Using a more robust synchronization mechanism (e.g., waiting for a specific event or state)
- Ensuring the delay is sufficient across different device performance levels
Suggested improvement: add explanatory comment or remove if unnecessary
yield fork(fetchUsersRoles); - yield delay(1000); + // TODO: Document why this delay is needed or replace with proper synchronization + yield delay(1000); yield fork(startVoipFork);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
app/sagas/login.jsapp/views/CallView/components/CallActionButton.test.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- app/views/CallView/components/CallActionButton.test.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
app/sagas/login.js (2)
app/lib/services/voip/MediaSessionInstance.ts (1)
mediaSessionInstance(187-187)app/lib/services/voip/simulateCall.ts (1)
simulateCall(28-106)
🔇 Additional comments (2)
app/sagas/login.js (2)
5-6: LGTM!The imports for
RNCallKeepandPermissionsAndroidare appropriate for the VOIP integration.
44-45: LGTM!The VOIP-related imports align with the new initialization logic.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
Android Build Available Rocket.Chat Experimental 4.69.0.108136 Internal App Sharing: https://play.google.com/apps/test/RQVpXLytHNc/ahAO29uNSZyIJxuUHVw5xneNQoB90tJlcdPj3KC9EZAC0RFVhEwZ8o0Z9WbXA3ZPewi90iGdqlNLQy5AUAzrCdwkFU |
|
iOS Build Available Rocket.Chat Experimental 4.69.0.108137 |
- 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.
diegolmello
force-pushed
the
feat.voip-lib-new
branch
from
c5e272e to
9b28770
Compare
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
147-149: Resolve version mismatch in ua-parser-js resolution.The project directly depends on
ua-parser-js@1.0.33, but the resolutions field overrides it to1.0.2. Additionally, the TypeScript types (@types/ua-parser-js@^0.7.36) are for the 0.7.x version line, creating a type/implementation mismatch. Update the resolution to1.0.33to align with the direct dependency and types, or investigate if1.0.2is specifically required and update the types accordingly.
🤖 Fix all issues with AI agents
In `@app/lib/methods/enterpriseModules.ts`:
- Around line 65-68: The function isVoipModuleAvailable currently returns a
hardcoded true which bypasses license checks; replace the hardcoded return with
a proper check using reduxStore.getState().enterpriseModules (e.g., verify
enterpriseModules is an array and return
enterpriseModules.includes('teams-voip') or a boolean equivalent) so the
presence of the 'teams-voip' module controls availability instead of always
enabling VoIP.
In `@app/lib/services/voip/getInitialEvents.ts`:
- Around line 18-54: getInitialEvents currently registers
VoipPushNotification.addEventListener and RNCallKeep.addEventListener every time
it's called, causing duplicate handlers; refactor by extracting listener
registration into a single-run setup (e.g., create setupListeners and a
module-scoped listenersRegistered flag) or store the returned listener
references and call corresponding removeEventListener when cleaning up, and then
call setupListeners from getInitialEvents instead of re-adding handlers;
reference VoipPushNotification.addEventListener, RNCallKeep.addEventListener,
getInitialEvents, listenersRegistered, and setupListeners in your changes.
- Around line 39-50: The code dispatches voipCallOpen when voipPushData matches
but never clears voipPushData, allowing duplicate dispatches; after the
successful store.dispatch(voipCallOpen({...})) call in the live answer handler
inside getInitialEvents (the branch that checks voipPushData.callUUID and
voipPushData.host), set voipPushData to null/undefined (or otherwise reset the
variable) immediately after dispatch to prevent reuse; ensure the assignment
uses the same voipPushData identifier in that scope so subsequent events won’t
re-trigger the same dispatch.
- Around line 69-81: The initial-event handler in getInitialEvents dispatches
voipCallOpen but does not clear the voipPushData, which can leave stale data;
after successful dispatch and before returning true, reset/clear the
voipPushData reference (the same way the live listener does) and then call
RNCallKeep.clearInitialEvents(); update the block handling voipPushData.host
(around the voipCallOpen dispatch) to clear voipPushData so subsequent logic
won’t reuse stale values.
In `@ios/AppDelegate.swift`:
- Around line 113-133: In
pushRegistry(_:didReceiveIncomingPushWith:for:completion:), ensure the optional
caller string is replaced with a non-nil fallback before calling
RNCallKeep.reportNewIncomingCall: if caller is nil use a sensible default (e.g.,
"Unknown" or the callId string) and pass that fallback for both the handle and
localizedCallerName; update references in this method (callId, caller,
CallIdUUID.generateUUIDv5, RNCallKeep.reportNewIncomingCall) so CallKit never
receives a nil handle.
In `@package.json`:
- Around line 144-145: The package.json currently adds "zustand": "^5.0.10"
which has known React 19 selector edge-cases; update package management and docs
to mitigate this by either pinning to a specific tested fixed version of zustand
(replace "^5.0.10" with the exact working version), add a note in the README or
migration guide describing the React 19 selector/autogenerated-selector
workarounds, and add a small compatibility test (exercise store selectors used
by the VoIP call store) to CI to catch selector regressions; reference the
"zustand" entry in package.json and the VoIP call store selectors when
implementing these changes.
♻️ Duplicate comments (4)
package.json (2)
50-50: Critical: Local filesystem path breaks CI and other developers' builds.This issue was previously flagged. The
@rocket.chat/media-signalingdependency references a local tgz file that may not be available in CI environments or for other developers.
95-95: Address knownreact-native-callkeep@4.3.16compatibility issues with React Native 0.79.This issue was previously flagged regarding reported runtime and behavior problems on both Android and iOS with React Native 0.79.
ios/CallIdUUID.m (1)
1-7: Duplicate ofios/Libraries/CallIdUUID.m— remove one.This file is identical to
ios/Libraries/CallIdUUID.m. Only one Objective-C bridge file should exist for theCallIdUUIDmodule. Keep the one that matches the Swift implementation location and remove the other to avoid duplicate symbol errors.app/sagas/login.js (1)
258-259: Await RNCallKeep setup promises.
On Line 258-259,RNCallKeep.setup()andRNCallKeep.canMakeMultipleCalls()return Promises; without yielding, the saga may proceed before setup completes.🛠️ Proposed fix
- RNCallKeep.setup(options); - RNCallKeep.canMakeMultipleCalls(false); + yield call([RNCallKeep, RNCallKeep.setup], options); + yield call([RNCallKeep, RNCallKeep.canMakeMultipleCalls], false);
🧹 Nitpick comments (4)
app/sagas/deepLinking.js (3)
308-314: Replaceconsole.logwith the existinglogutility for consistency.The file imports
logfrom../lib/methods/helpers/log(Line 19) which is used elsewhere in this file for error logging. Consider using a structured logging approach for VoIP debug statements or removing them before production release.
316-328: Consider extracting host normalization into a shared helper.This host URL normalization logic is duplicated in
handleOpen(lines 148-161) andhandleClickCallPush(lines 253-255). Extracting it into a reusable helper would reduce duplication.Example helper extraction
const normalizeHost = (host) => { if (!/^(http|https)/.test(host)) { host = /^localhost(:\d+)?/.test(host) ? `http://${host}` : `https://${host}`; } else { host = host.replace('http://', 'https://'); } return host.endsWith('/') ? host.slice(0, -1) : host; };
363-365: Consider user feedback when the server is not registered.When the server is not registered, the function silently logs and returns. Unlike
handleClickCallPushwhich callsfallbackNavigation(), this could leave the app in an indeterminate state. Consider either:
- Calling
fallbackNavigation()to ensure the app initializes properly.- Ending the CallKit call with an appropriate failure reason so the user isn't left with a stuck call UI.
app/lib/services/voip/getInitialEvents.ts (1)
93-96: Consider using the app'slogutility for error reporting.Using
console.errordirectly may miss integration with the app's error tracking/logging infrastructure. The app typically uses alogutility for consistency.
| export function isVoipModuleAvailable() { | ||
| const { enterpriseModules } = reduxStore.getState(); | ||
| return true; // enterpriseModules.includes('teams-voip'); | ||
| } |
There was a problem hiding this comment.
Hardcoded return true bypasses the enterprise license check.
The function always returns true, ignoring the actual enterprise module check. This appears to be development/testing code that should not be merged. If shipped, all users would have VoIP enabled regardless of their license.
Proposed fix to enable the license check
export function isVoipModuleAvailable() {
const { enterpriseModules } = reduxStore.getState();
- return true; // enterpriseModules.includes('teams-voip');
+ return enterpriseModules.includes('teams-voip');
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function isVoipModuleAvailable() { | |
| const { enterpriseModules } = reduxStore.getState(); | |
| return true; // enterpriseModules.includes('teams-voip'); | |
| } | |
| export function isVoipModuleAvailable() { | |
| const { enterpriseModules } = reduxStore.getState(); | |
| return enterpriseModules.includes('teams-voip'); | |
| } |
🧰 Tools
🪛 ESLint
[error] 66-66: 'enterpriseModules' is assigned a value but never used.
(@typescript-eslint/no-unused-vars)
🤖 Prompt for AI Agents
In `@app/lib/methods/enterpriseModules.ts` around lines 65 - 68, The function
isVoipModuleAvailable currently returns a hardcoded true which bypasses license
checks; replace the hardcoded return with a proper check using
reduxStore.getState().enterpriseModules (e.g., verify enterpriseModules is an
array and return enterpriseModules.includes('teams-voip') or a boolean
equivalent) so the presence of the 'teams-voip' module controls availability
instead of always enabling VoIP.
| VoipPushNotification.addEventListener('didLoadWithEvents', events => { | ||
| if (!events || !Array.isArray(events)) return; | ||
|
|
||
| for (const event of events) { | ||
| const { name, data } = event; | ||
|
|
||
| if (name === VoipPushNotification.RNVoipPushRemoteNotificationReceivedEvent) { | ||
| const voipData = data as { callId?: string; caller?: string; host?: string }; | ||
| const { callId, caller, host } = voipData; | ||
| if (callId) { | ||
| const callUUID = CallIdUUIDModule.toUUID(callId); | ||
| voipPushData = { callId, caller: caller || 'Unknown', host, callUUID }; | ||
| console.log('[VoIP][getInitialEvents] Stored VoIP push data:', voipPushData); | ||
| } | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| RNCallKeep.addEventListener('answerCall', ({ callUUID }) => { | ||
| console.log('[CallKeep][getInitialEvents] Live answerCall event received:', callUUID); | ||
|
|
||
| if (voipPushData && voipPushData.callUUID.toLowerCase() === callUUID.toLowerCase()) { | ||
| console.log('[CallKeep][getInitialEvents] Matched live answer to VoIP call'); | ||
| if (voipPushData.host) { | ||
| store.dispatch( | ||
| voipCallOpen({ | ||
| callId: voipPushData.callId, | ||
| callUUID: voipPushData.callUUID, | ||
| host: voipPushData.host | ||
| }) | ||
| ); | ||
| console.log('[CallKeep][getInitialEvents] Dispatched voipCallOpen action from live listener'); | ||
| } | ||
| } else { | ||
| console.log('[CallKeep][getInitialEvents] Live answer UUID mismatch. Answer:', callUUID, 'VoIP:', voipPushData?.callUUID); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Event listeners are registered but never removed, causing potential memory leaks and duplicate handlers.
VoipPushNotification.addEventListener and RNCallKeep.addEventListener are called inside getInitialEvents(), but no corresponding removeEventListener is ever called. If getInitialEvents() is invoked multiple times (e.g., on app restart or re-initialization), duplicate listeners will accumulate.
Consider:
- Registering these listeners once at module initialization, outside the function.
- Storing listener references and providing a cleanup mechanism.
- Using a flag to prevent re-registration.
Suggested approach: separate listener setup from initial event processing
let listenersRegistered = false;
const setupListeners = () => {
if (listenersRegistered) return;
listenersRegistered = true;
VoipPushNotification.addEventListener('didLoadWithEvents', events => {
// ... existing handler
});
RNCallKeep.addEventListener('answerCall', ({ callUUID }) => {
// ... existing handler
});
};
export const getInitialEvents = async (): Promise<boolean> => {
if (!isIOS) return false;
try {
setupListeners();
// ... rest of initial event processing
} catch (error) {
// ...
}
};🤖 Prompt for AI Agents
In `@app/lib/services/voip/getInitialEvents.ts` around lines 18 - 54,
getInitialEvents currently registers VoipPushNotification.addEventListener and
RNCallKeep.addEventListener every time it's called, causing duplicate handlers;
refactor by extracting listener registration into a single-run setup (e.g.,
create setupListeners and a module-scoped listenersRegistered flag) or store the
returned listener references and call corresponding removeEventListener when
cleaning up, and then call setupListeners from getInitialEvents instead of
re-adding handlers; reference VoipPushNotification.addEventListener,
RNCallKeep.addEventListener, getInitialEvents, listenersRegistered, and
setupListeners in your changes.
| if (voipPushData && voipPushData.callUUID.toLowerCase() === callUUID.toLowerCase()) { | ||
| console.log('[CallKeep][getInitialEvents] Matched live answer to VoIP call'); | ||
| if (voipPushData.host) { | ||
| store.dispatch( | ||
| voipCallOpen({ | ||
| callId: voipPushData.callId, | ||
| callUUID: voipPushData.callUUID, | ||
| host: voipPushData.host | ||
| }) | ||
| ); | ||
| console.log('[CallKeep][getInitialEvents] Dispatched voipCallOpen action from live listener'); | ||
| } |
There was a problem hiding this comment.
Clear voipPushData after successful dispatch to prevent stale data reuse.
After dispatching voipCallOpen in the live answerCall handler, voipPushData is not cleared. If another event arrives with the same UUID (e.g., from a retry or race condition), it could trigger a duplicate dispatch.
Proposed fix
if (voipPushData.host) {
store.dispatch(
voipCallOpen({
callId: voipPushData.callId,
callUUID: voipPushData.callUUID,
host: voipPushData.host
})
);
console.log('[CallKeep][getInitialEvents] Dispatched voipCallOpen action from live listener');
+ voipPushData = null;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (voipPushData && voipPushData.callUUID.toLowerCase() === callUUID.toLowerCase()) { | |
| console.log('[CallKeep][getInitialEvents] Matched live answer to VoIP call'); | |
| if (voipPushData.host) { | |
| store.dispatch( | |
| voipCallOpen({ | |
| callId: voipPushData.callId, | |
| callUUID: voipPushData.callUUID, | |
| host: voipPushData.host | |
| }) | |
| ); | |
| console.log('[CallKeep][getInitialEvents] Dispatched voipCallOpen action from live listener'); | |
| } | |
| if (voipPushData && voipPushData.callUUID.toLowerCase() === callUUID.toLowerCase()) { | |
| console.log('[CallKeep][getInitialEvents] Matched live answer to VoIP call'); | |
| if (voipPushData.host) { | |
| store.dispatch( | |
| voipCallOpen({ | |
| callId: voipPushData.callId, | |
| callUUID: voipPushData.callUUID, | |
| host: voipPushData.host | |
| }) | |
| ); | |
| console.log('[CallKeep][getInitialEvents] Dispatched voipCallOpen action from live listener'); | |
| voipPushData = null; | |
| } |
🤖 Prompt for AI Agents
In `@app/lib/services/voip/getInitialEvents.ts` around lines 39 - 50, The code
dispatches voipCallOpen when voipPushData matches but never clears voipPushData,
allowing duplicate dispatches; after the successful
store.dispatch(voipCallOpen({...})) call in the live answer handler inside
getInitialEvents (the branch that checks voipPushData.callUUID and
voipPushData.host), set voipPushData to null/undefined (or otherwise reset the
variable) immediately after dispatch to prevent reuse; ensure the assignment
uses the same voipPushData identifier in that scope so subsequent events won’t
re-trigger the same dispatch.
| if (voipPushData.host) { | ||
| store.dispatch( | ||
| voipCallOpen({ | ||
| callId: voipPushData.callId, | ||
| callUUID: voipPushData.callUUID, | ||
| host: voipPushData.host | ||
| }) | ||
| ); | ||
| console.log('[CallKeep][getInitialEvents] Dispatched voipCallOpen action'); | ||
|
|
||
| RNCallKeep.clearInitialEvents(); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Also clear voipPushData here after successful initial event handling.
Similar to the live listener, voipPushData should be cleared after successfully dispatching from initial events to prevent stale data issues.
Proposed fix
if (voipPushData.host) {
store.dispatch(
voipCallOpen({
callId: voipPushData.callId,
callUUID: voipPushData.callUUID,
host: voipPushData.host
})
);
console.log('[CallKeep][getInitialEvents] Dispatched voipCallOpen action');
+ voipPushData = null;
RNCallKeep.clearInitialEvents();
return true;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (voipPushData.host) { | |
| store.dispatch( | |
| voipCallOpen({ | |
| callId: voipPushData.callId, | |
| callUUID: voipPushData.callUUID, | |
| host: voipPushData.host | |
| }) | |
| ); | |
| console.log('[CallKeep][getInitialEvents] Dispatched voipCallOpen action'); | |
| RNCallKeep.clearInitialEvents(); | |
| return true; | |
| } | |
| if (voipPushData.host) { | |
| store.dispatch( | |
| voipCallOpen({ | |
| callId: voipPushData.callId, | |
| callUUID: voipPushData.callUUID, | |
| host: voipPushData.host | |
| }) | |
| ); | |
| console.log('[CallKeep][getInitialEvents] Dispatched voipCallOpen action'); | |
| voipPushData = null; | |
| RNCallKeep.clearInitialEvents(); | |
| return true; | |
| } |
🤖 Prompt for AI Agents
In `@app/lib/services/voip/getInitialEvents.ts` around lines 69 - 81, The
initial-event handler in getInitialEvents dispatches voipCallOpen but does not
clear the voipPushData, which can leave stale data; after successful dispatch
and before returning true, reset/clear the voipPushData reference (the same way
the live listener does) and then call RNCallKeep.clearInitialEvents(); update
the block handling voipPushData.host (around the voipCallOpen dispatch) to clear
voipPushData so subsequent logic won’t reuse stale values.
| public func pushRegistry(_ registry: PKPushRegistry, didReceiveIncomingPushWith payload: PKPushPayload, for type: PKPushType, completion: @escaping () -> Void) { | ||
| let callId = payload.dictionaryPayload["callId"] as? String | ||
| let caller = payload.dictionaryPayload["caller"] as? String | ||
|
|
||
| guard let callId = callId else { | ||
| completion() | ||
| return | ||
| } | ||
|
|
||
| // Convert callId to deterministic UUID v5 for CallKit | ||
| let callIdUUID = CallIdUUID.generateUUIDv5(from: callId) | ||
|
|
||
| RNVoipPushNotificationManager.didReceiveIncomingPush(with: payload, forType: type.rawValue) | ||
|
|
||
| RNCallKeep.reportNewIncomingCall( | ||
| callIdUUID, | ||
| handle: caller, | ||
| handleType: "generic", | ||
| hasVideo: true, | ||
| localizedCallerName: caller, | ||
| supportsHolding: true, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and read the AppDelegate.swift file
fd -n "AppDelegate.swift" --type fRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 212
🌐 Web query:
RNCallKeep reportNewIncomingCall nil handle parameter
💡 Result:
Short answer: reportNewIncomingCall’s handle argument must be a non-nil string — if you pass nil (or omit it) CallKit/CallKeep won’t show a proper incoming call and you may see failures. Fix by supplying a fallback handle (e.g. "unknown" or caller number/email) when your push payload doesn’t include one, and call reportNewIncomingCall from your PushKit handler (or use displayIncomingCall/startCall APIs) after the RN bridge/CallKeep setup is ready. [1][2][3]
Pointers / checklist:
- Provide a valid uuid and a non-nil handle string (e.g. "unknown"). [1][2]
- Use handleType ("generic"/"number"/"email") matching the handle. [10]
- If app cold-starts from a VoIP push, initialize RN bridge / wait briefly before emitting JS or calling methods from JS (or call native reportNewIncomingCall inside AppDelegate with a safe fallback handle). Many examples use dispatch_after / a small delay or call reportNewIncomingCall directly in pushRegistry. [2][3][5]
References:
[1] react-native-callkeep README / usage examples.
[2] Issues and examples showing reporting incoming call from PushKit (AppDelegate pushRegistry).
[3] Discussion/examples about cold-start/bridge timing and using fallback handle.
🌐 Web query:
CallKit CXCallUpdate nil caller name handle
💡 Result:
Short answer: If CXCallUpdate.remoteHandle is nil, CallKit will show the caller as "Unknown" (and the call row won’t be pressable in Recents). To control the displayed name set either remoteHandle (CXHandle) or set CXCallUpdate.localizedCallerName. For outgoing calls also supply a CXStartCallAction with a handle and/or report an updated CXCallUpdate so the Recents entry contains the handle/name. [1][2][3]
Useful snippets:
-
Set a handle:
let update = CXCallUpdate()
update.remoteHandle = CXHandle(type: .phoneNumber, value: phoneNumber)
provider.reportNewIncomingCall(with: uuid, update: update) { _ in } -
Or set only the displayed name:
update.localizedCallerName = "Alice"
Sources:
- CXCallUpdate docs (Apple). [CXCallUpdate]. [2]
- localizedCallerName docs (Apple). [1]
- Community/StackOverflow: missing handle => "Unknown" and not pressable in Recents. [3]
🌐 Web query:
RNVoipPushNotificationManager RNCallKeep iOS implementation
💡 Result:
Summary + minimal steps to implement RNVoipPushNotificationManager with RNCallKeep on iOS:
-
Add capabilities (Xcode):
- Background Modes -> Voice over IP enabled
- Push Notifications enabled. [1][2]
-
Install & link:
- react-native-voip-push-notification (pod install) and react-native-callkeep. [1][2]
-
AppDelegate: import PushKit, RNVoipPushNotificationManager and RNCallKeep and implement PKPushRegistryDelegate. In Objective‑C you must forward PushKit events to the RN module and report the incoming call to CallKit immediately when a VoIP push arrives. Example (minimal, place in AppDelegate.m):
-
(void)pushRegistry:(PKPushRegistry *)registry didUpdatePushCredentials:(PKPushCredentials *)credentials forType:(PKPushType)type {
[RNVoipPushNotificationManager didUpdatePushCredentials:credentials forType:(NSString *)type];
} -
(void)pushRegistry:(PKPushRegistry *)registry didInvalidatePushTokenForType:(PKPushType)type {
// handle invalidation if needed
} -
(void)pushRegistry:(PKPushRegistry *)registry didReceiveIncomingPushWithPayload:(PKPushPayload *)payload forType:(PKPushType)type withCompletionHandler:(void(^)(void))completion {
// 1) forward to RN module so JS listeners receive it
[RNVoipPushNotificationManager didReceiveIncomingPushWithPayload:payload forType:(NSString *)type];
// 2) report call to CallKit ASAP (required on iOS 13+)
// either call native RNCallKeep APIs or trigger JS to call RNCallKeep.reportNewIncomingCall
// e.g. notify RNCallKeep/JS to reportNewIncomingCall(...)
// 3) store and call completion when handled by JS
[RNVoipPushNotificationManager addCompletionHandler:payload.dictionaryPayload[@"uuid"] completionHandler:completion];
}
Also implement code to remove completion handlers when JS calls VoipPushNotification.onVoipNotificationCompleted(uuid). [1][2]
Important: On iOS 13+ Apple will terminate your app if you receive a VoIP push and do not report a CallKit incoming call quickly. Always call reportNewIncomingCall (CallKit) immediately on receiving the push. [1][3]
-
-
JS side:
- Register for VoIP token and send token to your server.
- Listen for 'notification' event from react-native-voip-push-notification to show UI/prepare media.
- Use react-native-callkeep to reportNewIncomingCall(...) when receiving the push (or have native call into RNCallKeep). Add RNCallKeep event listeners for answerCall/endCall to start/stop the call flow. After handling, call VoipPushNotification.onVoipNotificationCompleted(uuid) to run the completion handler. [1][2]
References:
- react-native-voip-push-notification README (native APIs & usage). [1]
- npm package docs / installation notes and capability reminders. [2]
- Apple / CallKit requirement (must report incoming call ASAP) and examples discussed in integrations (CallKit/VoIP guidance). [3]
If you want, I can paste a copy-pasteable Obj‑C AppDelegate.m snippet and the corresponding JS event wiring for RNCallKeep + RNVoipPushNotificationManager.
Handle missing caller to prevent CallKit failure.
When caller is nil (line 115), passing it to handle in reportNewIncomingCall will cause CallKit to fail to properly display the incoming call. Provide a fallback value.
🛠️ Proposed fix
let caller = payload.dictionaryPayload["caller"] as? String
guard let callId = callId else {
completion()
return
}
+
+ let callerName = caller ?? "Unknown"
// Convert callId to deterministic UUID v5 for CallKit
let callIdUUID = CallIdUUID.generateUUIDv5(from: callId)
RNVoipPushNotificationManager.didReceiveIncomingPush(with: payload, forType: type.rawValue)
RNCallKeep.reportNewIncomingCall(
callIdUUID,
- handle: caller,
+ handle: callerName,
handleType: "generic",
hasVideo: true,
- localizedCallerName: caller,
+ localizedCallerName: callerName,
supportsHolding: true,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public func pushRegistry(_ registry: PKPushRegistry, didReceiveIncomingPushWith payload: PKPushPayload, for type: PKPushType, completion: @escaping () -> Void) { | |
| let callId = payload.dictionaryPayload["callId"] as? String | |
| let caller = payload.dictionaryPayload["caller"] as? String | |
| guard let callId = callId else { | |
| completion() | |
| return | |
| } | |
| // Convert callId to deterministic UUID v5 for CallKit | |
| let callIdUUID = CallIdUUID.generateUUIDv5(from: callId) | |
| RNVoipPushNotificationManager.didReceiveIncomingPush(with: payload, forType: type.rawValue) | |
| RNCallKeep.reportNewIncomingCall( | |
| callIdUUID, | |
| handle: caller, | |
| handleType: "generic", | |
| hasVideo: true, | |
| localizedCallerName: caller, | |
| supportsHolding: true, | |
| public func pushRegistry(_ registry: PKPushRegistry, didReceiveIncomingPushWith payload: PKPushPayload, for type: PKPushType, completion: `@escaping` () -> Void) { | |
| let callId = payload.dictionaryPayload["callId"] as? String | |
| let caller = payload.dictionaryPayload["caller"] as? String | |
| guard let callId = callId else { | |
| completion() | |
| return | |
| } | |
| let callerName = caller ?? "Unknown" | |
| // Convert callId to deterministic UUID v5 for CallKit | |
| let callIdUUID = CallIdUUID.generateUUIDv5(from: callId) | |
| RNVoipPushNotificationManager.didReceiveIncomingPush(with: payload, forType: type.rawValue) | |
| RNCallKeep.reportNewIncomingCall( | |
| callIdUUID, | |
| handle: callerName, | |
| handleType: "generic", | |
| hasVideo: true, | |
| localizedCallerName: callerName, | |
| supportsHolding: true, |
🤖 Prompt for AI Agents
In `@ios/AppDelegate.swift` around lines 113 - 133, In
pushRegistry(_:didReceiveIncomingPushWith:for:completion:), ensure the optional
caller string is replaced with a non-nil fallback before calling
RNCallKeep.reportNewIncomingCall: if caller is nil use a sensible default (e.g.,
"Unknown" or the callId string) and pass that fallback for both the handle and
localizedCallerName; update references in this method (callId, caller,
CallIdUUID.generateUUIDv5, RNCallKeep.reportNewIncomingCall) so CallKit never
receives a nil handle.
|
iOS Build Available Rocket.Chat Experimental 4.69.0.108195 |
diegolmello
force-pushed
the
feat.voip-lib-new
branch
from
33fe076 to
aa3ca88
Compare
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/lib/services/restApi.ts`:
- Around line 1013-1018: Only register the VoIP push token when a real token
exists: call getVoipPushToken(), check it's non-null/non-empty, and only then
call sdk.post('push.token') with value set to the token and appName from
getBundleId(); do not send an empty string. Also replace or verify the hardcoded
type 'gcm'—use the appropriate type for VoIP on iOS (e.g., 'apn') or make the
type configurable so sdk.post('push.token') uses the correct token type for
VoIP.
♻️ Duplicate comments (4)
patches/react-native-callkeep+4.3.16.patch (1)
9-16: Confirm all JS/TS call sites use the 4‑arg signature (hasVideo).
Removing the 3‑argdisplayIncomingCall/startCalloverloads is a breaking change if any callers still use the old signature. Please verify all call sites now pass the booleanhasVideo.Run this to confirm usage and argument counts:
#!/bin/bash # Inspect call sites for displayIncomingCall/startCall signatures rg -nP '\bdisplayIncomingCall\s*\(' -C 3 --type ts --type tsx --type js --type jsx rg -nP '\bstartCall\s*\(' -C 3 --type ts --type tsx --type js --type jsxAlso applies to: 21-28
app/lib/services/voip/getInitialEvents.ts (3)
21-46: Listener registration still lacks cleanup mechanism.As noted in previous review, event listeners are registered inside
getInitialEvents()without removal or guard against re-registration. If called multiple times, duplicate handlers will accumulate.
48-66:voipPushDatastill not cleared after dispatch in live listener.As noted in previous review, after successfully dispatching
voipCallOpen,voipPushDatashould be cleared to prevent stale data reuse on subsequent events.
81-93:voipPushDatastill not cleared after initial event dispatch.As noted in previous review,
voipPushDatashould be cleared after successfully dispatching from initial events to prevent stale data issues.
🧹 Nitpick comments (1)
app/lib/services/voip/getInitialEvents.ts (1)
41-44: Consider validatingdatatype before assignment.At line 42,
datafrom the event is assigned directly tovoipPushTokenwithout type validation. If the event payload is malformed, this could store an unexpected value.Suggested defensive check
if (name === VoipPushNotification.RNVoipPushRemoteNotificationsRegisteredEvent) { - voipPushToken = data; + voipPushToken = typeof data === 'string' ? data : null; console.log('[VoIP][getInitialEvents] Registered VoIP push token:', voipPushToken); }
| // TODO: this is temp only for VoIP push token | ||
| await sdk.post('push.token', { | ||
| type: 'gcm', | ||
| value: getVoipPushToken() || '', | ||
| appName: getBundleId | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the file
git ls-files app/lib/services/restApi.tsRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 105
🏁 Script executed:
# Check if the file exists and get its size
wc -l app/lib/services/restApi.tsRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 110
🏁 Script executed:
# Read the specific lines mentioned (1013-1018) with surrounding context
sed -n '1000,1030p' app/lib/services/restApi.tsRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 743
🏁 Script executed:
# Search for other VoIP push token registrations to understand the pattern
rg -i "voip.*push|push.*voip" --type ts -B 3 -A 3Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 8301
🏁 Script executed:
# Search for push.token API calls to see how it's used elsewhere
rg "sdk\.post.*push\.token" --type ts -B 3 -A 3Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 768
🏁 Script executed:
# Check for platform checks (iOS/Android) around the code
rg -i "isios|ios|android|platform" app/lib/services/restApi.ts -B 2 -A 2 | head -50Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 535
🏁 Script executed:
# Look for the getVoipPushToken function to understand when it returns values
rg "getVoipPushToken|function getVoipPushToken" --type ts -B 3 -A 8Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 2212
🏁 Script executed:
# Check if there are any platform-specific conditionals in the file around line 1013
sed -n '980,1040p' app/lib/services/restApi.tsRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 1340
🏁 Script executed:
# Search for all push token type references in the codebase
rg "type.*['\"]apn|type.*['\"]gcm|type.*['\"]voip" --type ts -B 2 -A 2Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 652
🏁 Script executed:
# Look for any comments or documentation about push types
rg -i "push.*type|apn|voip.*push" --type ts | grep -i "type" | head -20Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 606
🏁 Script executed:
# Check if there are any other VoIP-related registrations or configurations
rg -i "voippush|voip" --type ts -A 3 -B 1 | head -60Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 4289
🏁 Script executed:
# Search for any server version checks or API documentation references related to push
rg -i "push.*0\.60|0\.60.*push" --type tsRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 60
🏁 Script executed:
# Check if there's any 'voip' type reference anywhere in the codebase
rg "voip" --type ts --type json --type md | grep -i "type" | head -20Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 60
🏁 Script executed:
# Look at the full context of registerPushToken to understand the intent
sed -n '990,1045p' app/lib/services/restApi.tsRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 1276
🏁 Script executed:
# Search for any related issues, PRs, or documentation in comments
rg -i "TODO|FIXME|NOTE" app/lib/services/restApi.ts | head -10Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 338
🏁 Script executed:
# Check if there's a changelog or any indication of what push types are supported
git log --oneline --all -- app/lib/services/restApi.ts | head -20Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 179
🏁 Script executed:
# Get the full commit message and details for the VoIP push token commit
git show aa3ca889 --statRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 50391
🏁 Script executed:
# Check if there are any follow-up commits that address this TODO
git log --oneline --all -- app/lib/services/restApi.ts | grep -i "voip\|push" | head -10Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 123
🏁 Script executed:
# Search for any issue references in commit history related to VoIP push
git log --all --grep="voip" --grep="push" --oneline | head -20Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 123
VoIP push token registration needs guards against empty submissions.
The TODO confirms this is a temporary implementation. The two main issues are:
-
Empty token submissions: When
getVoipPushToken()returnsnull, an empty string is sent to the server, creating unnecessary network traffic. Guard this call to avoid registering empty tokens. -
Type hardcoded as
'gcm': While acknowledged as temporary (per the TODO comment), consider whether this should be'apn'for iOS consistency with the main token registration, or verify if the server expects a different type for VoIP tokens.
Add a guard to only register when a token is actually available:
Suggested fix
- // TODO: this is temp only for VoIP push token
- await sdk.post('push.token', {
- type: 'gcm',
- value: getVoipPushToken() || '',
- appName: getBundleId
- });
+ // TODO: this is temp only for VoIP push token
+ const voipToken = getVoipPushToken();
+ if (voipToken) {
+ await sdk.post('push.token', {
+ type: 'gcm',
+ value: voipToken,
+ appName: getBundleId
+ });
+ }🤖 Prompt for AI Agents
In `@app/lib/services/restApi.ts` around lines 1013 - 1018, Only register the VoIP
push token when a real token exists: call getVoipPushToken(), check it's
non-null/non-empty, and only then call sdk.post('push.token') with value set to
the token and appName from getBundleId(); do not send an empty string. Also
replace or verify the hardcoded type 'gcm'—use the appropriate type for VoIP on
iOS (e.g., 'apn') or make the type configurable so sdk.post('push.token') uses
the correct token type for VoIP.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/lib/services/voip/getInitialEvents.ts`:
- Around line 39-42: The code in getInitialEvents.ts uses the event constant
VoipPushNotification.RNVoipPushRemoteNotificationsRegisteredEvent and passes
untyped data directly into setVoipPushToken(data); validate that the event
payload is a string token before calling setVoipPushToken: check typeof data ===
'string' and that it is non-empty (or extract token if data is an object with a
token property), otherwise log a warning/error and do not call setVoipPushToken
to avoid passing malformed values; keep the existing console.log for successful
token registration but include the validated token in the log.
♻️ Duplicate comments (3)
app/lib/services/voip/getInitialEvents.ts (3)
19-64: Event listeners are registered on every call but never removed.This concern was raised in a prior review and remains unaddressed. Each invocation of
getInitialEvents()adds new listeners without cleanup, causing memory leaks and duplicate handler executions. Consider extracting listener setup to a guarded one-time initialization.
49-60: ClearvoipPushDataafter successful dispatch in live listener.This was flagged in a prior review. After dispatching
voipCallOpen,voipPushDatashould be set tonullto prevent duplicate dispatches from race conditions or retries.
79-91: ClearvoipPushDataafter successful dispatch from initial events.This was flagged in a prior review. Similar to the live listener,
voipPushDatashould be set tonullafter dispatching to prevent stale data issues.
🧹 Nitpick comments (1)
app/lib/services/voip/pushTokenAux.ts (1)
4-8: Consider allowing the token to be cleared.The setter currently only accepts
string, so there's no way to reset the token tonull(e.g., on logout or when VoIP is disabled). If this is intentional because the app restarts on logout, this is fine. Otherwise, consider acceptingstring | nullor adding a separateclearVoipPushTokenfunction.♻️ Optional: Allow clearing the token
export const getVoipPushToken = (): string | null => voipPushToken; -export const setVoipPushToken = (token: string): void => { +export const setVoipPushToken = (token: string | null): void => { voipPushToken = token; };
| if (name === VoipPushNotification.RNVoipPushRemoteNotificationsRegisteredEvent) { | ||
| setVoipPushToken(data); | ||
| console.log('[VoIP][getInitialEvents] Registered VoIP push token:', data); | ||
| } |
There was a problem hiding this comment.
Missing type validation for push token data.
data from the event is untyped and passed directly to setVoipPushToken(string). If the event payload is malformed, this could cause unexpected behavior.
Proposed fix
if (name === VoipPushNotification.RNVoipPushRemoteNotificationsRegisteredEvent) {
- setVoipPushToken(data);
+ if (typeof data === 'string') {
+ setVoipPushToken(data);
+ }
console.log('[VoIP][getInitialEvents] Registered VoIP push token:', data);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (name === VoipPushNotification.RNVoipPushRemoteNotificationsRegisteredEvent) { | |
| setVoipPushToken(data); | |
| console.log('[VoIP][getInitialEvents] Registered VoIP push token:', data); | |
| } | |
| if (name === VoipPushNotification.RNVoipPushRemoteNotificationsRegisteredEvent) { | |
| if (typeof data === 'string') { | |
| setVoipPushToken(data); | |
| } | |
| console.log('[VoIP][getInitialEvents] Registered VoIP push token:', data); | |
| } |
🤖 Prompt for AI Agents
In `@app/lib/services/voip/getInitialEvents.ts` around lines 39 - 42, The code in
getInitialEvents.ts uses the event constant
VoipPushNotification.RNVoipPushRemoteNotificationsRegisteredEvent and passes
untyped data directly into setVoipPushToken(data); validate that the event
payload is a string token before calling setVoipPushToken: check typeof data ===
'string' and that it is non-empty (or extract token if data is an object with a
token property), otherwise log a warning/error and do not call setVoipPushToken
to avoid passing malformed values; keep the existing console.log for successful
token registration but include the validated token in the log.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/sagas/login.js`:
- Around line 265-276: startVoipFork currently assumes hasPermission(...)
returns an array and indexing hasPermissions[0]/[1] can throw when it returns
undefined; update the call to hasPermission in startVoipFork so you default the
result to an empty array (e.g. const hasPermissions = yield hasPermission([...])
|| [] or wrap with Array.isArray check) before checking hasPermissions[0] and
hasPermissions[1], so failures are treated as "no permission" and the rest of
the logic (isVoipModuleAvailable, initCallKeep, mediaSessionInstance.init)
behaves safely.
♻️ Duplicate comments (1)
app/sagas/login.js (1)
231-259: Await RNCallKeep setup to avoid races and unhandled rejections.
RNCallKeep.setup()andcanMakeMultipleCalls()return Promises; without yielding them, errors won’t be caught by the saga and setup can still be in flight when later calls run.🔧 Proposed fix
- RNCallKeep.setup(options); - RNCallKeep.canMakeMultipleCalls(false); + yield call([RNCallKeep, RNCallKeep.setup], options); + yield call([RNCallKeep, RNCallKeep.canMakeMultipleCalls], false);
| const startVoipFork = function* startVoipFork() { | ||
| try { | ||
| const allowInternalVoiceCallRoles = yield select(state => state.permissions['allow-internal-voice-calls']); | ||
| const allowExternalVoiceCallRoles = yield select(state => state.permissions['allow-external-voice-calls']); | ||
|
|
||
| const hasPermissions = yield hasPermission([allowInternalVoiceCallRoles, allowExternalVoiceCallRoles]); | ||
| if (isVoipModuleAvailable() && (hasPermissions[0] || hasPermissions[1])) { | ||
| yield call(initCallKeep); | ||
| const userId = yield select(state => state.login.user.id); | ||
| mediaSessionInstance.init(userId); | ||
| // simulateCall(); | ||
| } |
There was a problem hiding this comment.
Guard against undefined permission results.
hasPermission() can return undefined on error; indexing that throws (caught, but noisy) and always skips VoIP. Default to an empty array to treat failures as “no permission.”
✅ Suggested guard
- const hasPermissions = yield hasPermission([allowInternalVoiceCallRoles, allowExternalVoiceCallRoles]);
+ const hasPermissions = (yield hasPermission([allowInternalVoiceCallRoles, allowExternalVoiceCallRoles])) || [];
if (isVoipModuleAvailable() && (hasPermissions[0] || hasPermissions[1])) {🤖 Prompt for AI Agents
In `@app/sagas/login.js` around lines 265 - 276, startVoipFork currently assumes
hasPermission(...) returns an array and indexing hasPermissions[0]/[1] can throw
when it returns undefined; update the call to hasPermission in startVoipFork so
you default the result to an empty array (e.g. const hasPermissions = yield
hasPermission([...]) || [] or wrap with Array.isArray check) before checking
hasPermissions[0] and hasPermissions[1], so failures are treated as "no
permission" and the rest of the logic (isVoipModuleAvailable, initCallKeep,
mediaSessionInstance.init) behaves safely.
Proposed changes
Issue(s)
How to test or reproduce
Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit
New Features
Platform integration
Settings & Localization
Tests
Mocks
✏️ Tip: You can customize this high-level summary in your review settings.