feat: Voice support #6918
feat: Voice support #6918
Conversation
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
WalkthroughAdds comprehensive VoIP calling infrastructure across iOS, Android, and JavaScript, including native push notification handling, a Zustand-based call state store, new CallView UI components, platform-specific native modules (CallIdUUID, VoipModule), and integration with react-native-callkeep and WebRTC media signaling. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant PushService as Push<br/>Service
participant NativeModule as Android Native<br/>Module
participant JSBridge as React Native<br/>Bridge
participant CallStore as Call<br/>Store
participant UI as CallView<br/>UI
User->>PushService: VoIP Call Arrives
PushService->>NativeModule: Firebase FCM Message
NativeModule->>NativeModule: Parse VoipPayload
NativeModule->>NativeModule: Generate CallUUID
NativeModule->>NativeModule: Show Incoming Call Activity
NativeModule->>JSBridge: setupVoipEventListeners<br/>(emit VoipCallAction)
JSBridge->>CallStore: Dispatch voipCallOpen<br/>with callId, callUUID, host
CallStore->>CallStore: validateAndConnect<br/>to correct server
CallStore->>NativeModule: mediaSessionInstance<br/>.answerCall(callUUID)
NativeModule->>JSBridge: Initialize MediaSession<br/>(WebRTC)
JSBridge->>CallStore: setCall(mediaCall, callUUID)
CallStore->>UI: Notify subscribers
UI->>User: Display CallView
User->>UI: Tap Answer Button
UI->>CallStore: Action: toggleFocus
CallStore->>NativeModule: RNCallKeep<br/>.answerCall(callUUID)
sequenceDiagram
actor User
participant UI as CallView<br/>UI
participant CallStore as Call<br/>Store
participant MediaSession as MediaSession<br/>Instance
participant WebRTC as WebRTC<br/>Processor
participant RNCallKeep as RNCallKeep<br/>API
User->>UI: Tap Mute Button
UI->>CallStore: Action: toggleMute
CallStore->>MediaSession: call.setMuted(true)
MediaSession->>WebRTC: Update audio track
WebRTC->>WebRTC: Disable microphone
MediaSession->>CallStore: Update: isMuted = true
CallStore->>UI: Notify subscribers
UI->>UI: Re-render mute button
User->>UI: See updated mute state
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Rationale: Substantial multi-platform VoIP feature with 50+ files spanning Android native (Kotlin), iOS native (Swift/Objective-C), React Native TypeScript, state management, UI components, navigation, and native module integrations. High logic density across platform-specific implementations (Telecom framework, CallKit, CallKeep, WebRTC), complex event handling flows, and deep system integration requiring careful review of each platform's specifics. Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 [email protected] 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
[email protected], 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 known[email protected]compatibility 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); }
app/lib/services/restApi.ts
Outdated
| // 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.
|
iOS Build Available Rocket.Chat Experimental 4.69.0.108197 |
diegolmello
requested a deployment
to
approve_e2e_testing
GitHub Actions
Waiting
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Fix all issues with AI agents
In `@android/app/src/main/AndroidManifest.xml`:
- Around line 25-26: Update the two <uses-feature> entries for
android.hardware.audio.output and android.hardware.microphone so they don't
cause Play Store device filtering: modify the <uses-feature
android:name="android.hardware.audio.output" /> and <uses-feature
android:name="android.hardware.microphone" /> elements to include
android:required="false" (i.e., set the required attribute to false for both
features) so devices without those hardware features are not excluded.
In
`@android/app/src/main/java/chat/rocket/reactnative/notification/NotificationIntentHandler.kt`:
- Around line 43-68: In handleVoipIntent, ensure malformed VoIP intents (missing
event) don't leave stale state: when event is null, still clear the voip flag
and cancel the notification only if notificationId != 0 (avoid canceling ID 0),
but skip calling VoipModule.storePendingVoipCall/emitCallAnswered; always call
intent.removeExtra("voipAction") before returning and call
VoipNotification.cancelById(context, notificationId) only when notificationId !=
0; keep existing behavior for valid events (store pending call, cancel, emit on
"accept").
In
`@android/app/src/main/java/chat/rocket/reactnative/notification/RCFirebaseMessagingService.kt`:
- Around line 65-85: parseVoipPayload currently constructs a VoipPayload even
when required fields may be missing; add early-return guards that validate
required keys (at least callId, calleeId, caller, host — type is already
checked) by verifying data contains each key and that none are null or blank
(use isNullOrEmpty) and return null if any are invalid before constructing
VoipPayload; update the parseVoipPayload function to perform these checks so
downstream code never receives a payload with empty critical fields.
In
`@android/app/src/main/java/chat/rocket/reactnative/notification/VoipNotification.kt`:
- Around line 81-86: The DeclineReceiver BroadcastReceiver defined as class
DeclineReceiver (inside VoipNotification) is never registered in the Android
manifest so decline actions won't be delivered; register it by adding a
<receiver> entry for
chat.rocket.reactnative.notification.VoipNotification$DeclineReceiver inside the
<application> tag of AndroidManifest.xml and include an intent-filter for the
decline action name (the ACTION_VOIP_DECLINE string used by the notification)
and set exported="false" so Android can instantiate the receiver and
VoipNotification.handleDeclineAction(context, callUUID) will be invoked.
- Around line 281-284: The code in VoipNotification.kt retrieves ic_notification
via context.resources.getIdentifier and may get 0 if the drawable is missing;
update the smallIcon resolution (where smallIconResId is computed) to check for
0 and use a safe fallback (e.g., the app's launcher icon or a platform default
like android.R.drawable.sym_def_app_icon or context.applicationInfo.icon) before
passing it to NotificationBuilder so the notification always has a valid icon;
adjust any logic that uses smallIconResId (smallIconResId variable and the code
that sets the notification small icon) to use the resolved fallback value.
In
`@android/app/src/main/java/chat/rocket/reactnative/notification/VoipTurboPackage.kt`:
- Around line 9-31: The ReactModuleInfo instantiation in
VoipTurboPackage.getReactModuleInfoProvider is missing the hasConstants boolean;
update the ReactModuleInfo(...) call used for NativeVoipSpec.NAME (inside
VoipTurboPackage) to include a false value for hasConstants between the
needsEagerInit and isCxxModule parameters so the constructor receives seven
arguments (canOverrideExistingModule, needsEagerInit, hasConstants, isCxxModule,
isTurboModule).
In
`@android/app/src/main/java/chat/rocket/reactnative/voip/IncomingCallActivity.kt`:
- Around line 130-158: In handleAccept(), the notification cancellation uses
callUUID.replace("-", "").hashCode() which won't match the ID used when creating
the notification (VoipNotification uses callId.hashCode()); change the cancel
logic to compute the notificationId from callId (the same way VoipNotification
does) and call VoipNotification.cancelById(this, notificationId) so the IDs
match and the notification is actually removed (reference: handleAccept(),
callUUID, callId, VoipNotification.cancelById).
- Around line 42-83: The lock-screen API calls in onCreate are ordered
incorrectly: call setShowWhenLocked(true) and setTurnScreenOn(true) (and/or set
relevant window flags) before invoking
KeyguardManager.requestDismissKeyguard(...) so the activity is marked visible
over the lock screen prior to dismissal; move the
setShowWhenLocked/setTurnScreenOn block above the requestDismissKeyguard call
and pass a non-null KeyguardManager.KeyguardDismissCallback to
requestDismissKeyguard to handle dismissal success/failure and log or handle
errors.
In
`@android/app/src/main/java/chat/rocket/reactnative/voip/VoipForegroundService.kt`:
- Around line 104-116: The createNotification function may pass a zero resource
id when resources.getIdentifier("ic_notification", "drawable", packageName)
doesn't find the drawable; update createNotification to check smallIconResId and
use a safe fallback (e.g., android.R.drawable.sym_def_app_icon or the app's
launcher icon obtained via packageManager) before calling
NotificationCompat.Builder(this, CHANNEL_ID).setSmallIcon(...), ensuring
setSmallIcon always receives a valid non-zero drawable id (refer to
smallIconResId, createNotification, CHANNEL_ID, and NotificationCompat.Builder
to locate the change).
In `@android/app/src/main/res/layout/activity_incoming_call.xml`:
- Around line 1-69: Replace hardcoded user-visible strings and color hexes in
this layout with resource references: move texts ("Unknown", "Incoming call",
"Decline call", "Accept call", avatar contentDescription) into
res/values/strings.xml and replace android:text and android:contentDescription
on TextViews and ImageButtons (ids: caller_name, incoming_label, btn_decline,
btn_accept, caller_avatar) with `@string/`...; move colors (`#000000`, `#FFFFFF`,
`#CCCCCC`, `#F44336`, `#4CAF50`) into res/values/colors.xml and replace
android:background, android:textColor, android:backgroundTint attributes in the
root LinearLayout, caller_name, incoming_label, btn_decline and btn_accept with
`@color/`...; ensure the new resource names are descriptive and update all
attribute references in activity_incoming_call.xml accordingly.
In `@app/lib/native/NativeVoipAndroid.ts`:
- Around line 1-9: The current module lookup uses
TurboModuleRegistry.getEnforcing('VoipModule') which throws if the native module
isn't registered; change it to TurboModuleRegistry.get<Spec>('VoipModule') so
the lookup returns null on iOS instead of crashing—update the default export in
NativeVoipAndroid.ts to use get(...) (referenced Spec and TurboModuleRegistry)
so the existing null check in getInitialEvents.ts (guarding NativeVoipModule)
works as intended.
In `@app/lib/notifications/push.ts`:
- Around line 186-189: The console.log in push.ts is printing sensitive device
tokens; remove the log or guard it behind a development-only check.
Specifically, update the code around the deviceToken/token assignment (where
console.log('[push.ts] Registered for push notifications:', token) is called) to
either delete that console.log entirely or wrap it with a dev-only condition
(e.g., NODE_ENV === 'development' or a feature flag) or use a debug-level logger
that is disabled in production so deviceToken and token are never written to
production logs.
🧹 Nitpick comments (12)
index.js (1)
28-40: Consider localizing user‑facing strings.The permission dialog/foreground‑service strings are hardcoded; wiring them through i18n would avoid English‑only UI.
android/app/src/main/java/chat/rocket/reactnative/voip/IncomingCallActivity.kt (2)
85-93: TODO: avatar loading needs caller identity.
If you want, I can help plumb username/avatar URL into the intent and load via your existing image pipeline.
160-177: TODO: decline should notify server-side.
Right now only a local event fires; if the server needs an explicit decline to stop ringing, consider wiring that in. Happy to help.android/app/src/main/java/chat/rocket/reactnative/notification/NativeCallIdUUIDSpec.kt (1)
8-18: Consider converting to an async method to avoid JS thread blocking.
isBlockingSynchronousMethod = trueblocks the JavaScript thread until the native call returns, which React Native discourages. Even for lightweight operations like UUID transforms, prefer an async variant (Promise/callback) to maintain JS responsiveness and preserve Chrome remote debugging capabilities.android/app/src/main/java/chat/rocket/reactnative/voip/VoipForegroundService.kt (1)
15-15: Remove unused import.
VoipNotificationis imported but not used in this file.-import chat.rocket.reactnative.notification.VoipNotificationandroid/app/src/main/AndroidManifest.xml (1)
20-20:BIND_TELECOM_CONNECTION_SERVICEis not a user-granted permission.This permission is typically declared only in the
<service>element (which you have at line 138), not as a<uses-permission>. Apps cannot request this permission; only the system can bind to services protected by it. This line can be safely removed.- <uses-permission android:name="android.permission.BIND_TELECOM_CONNECTION_SERVICE" />android/app/src/main/java/chat/rocket/reactnative/notification/VoipModule.kt (4)
55-81: Remove commented-out code before merging.This commented-out
emitIncomingCallfunction andcancelIncomingCall(lines 141-153) should be removed if not needed, or implemented if required. Keeping dead code makes maintenance harder.
24-24:EVENT_INCOMING_CALLconstant is defined but never used.Consider removing this unused constant, or implement the functionality that would use it.
- private const val EVENT_INCOMING_CALL = "VoipIncomingCall"
121-136: Code duplication betweenclearPendingVoipCallandclearPendingVoipCallInternal.Both methods perform the same SharedPreferences clear operation. Consider having the companion method delegate to a shared implementation or use the instance method internally.
Proposed consolidation
`@JvmStatic` internal fun clearPendingVoipCall(context: Context) { try { context.getSharedPreferences(PREFS_NAME, Context.MODE_PRIVATE) .edit() .clear() .apply() Log.d(TAG, "Cleared pending VoIP call data") } catch (e: Exception) { Log.e(TAG, "Error clearing pending VoIP call", e) } } } // ... private fun clearPendingVoipCallInternal() { - try { - reactApplicationContext.getSharedPreferences(PREFS_NAME, Context.MODE_PRIVATE) - .edit() - .clear() - .apply() - Log.d(TAG, "Cleared pending VoIP call data") - } catch (e: Exception) { - Log.e(TAG, "Error clearing pending VoIP call", e) - } + clearPendingVoipCall(reactApplicationContext) }Also applies to: 238-248
159-189: Consider extracting redundant JSON data construction.The
storePendingVoipCallmethod stores the same data twice: once as a JSON string inpendingActionand again as individual SharedPreferences keys. If the JSON string is sufficient, the individual keys can be removed to reduce redundancy.android/app/src/main/java/chat/rocket/reactnative/notification/VoipNotification.kt (2)
166-170: Redundant null check on non-nullable parameter.
callUUIDis declared asString(non-nullable in Kotlin), soisNullOrEmpty()will never match the null case. UseisEmpty()instead, or consider if this check is needed at all since the caller already generates a valid UUID.- if (callUUID.isNullOrEmpty()) { + if (callUUID.isEmpty()) {
285-303: Remove dead code for avatar handling.
avatarBitmapis alwaysnull(line 286), so the conditional block at lines 301-303 will never execute. Either implement avatar loading or remove this dead code.Proposed removal
- // Avatar not available in VoipPayload format (would require caller username) - val avatarBitmap: Bitmap? = null - // Build notification val builder = NotificationCompat.Builder(context, CHANNEL_ID).apply { setSmallIcon(smallIconResId) // ... - - if (avatarBitmap != null) { - setLargeIcon(avatarBitmap) - }
| <uses-feature android:name="android.hardware.audio.output" /> | ||
| <uses-feature android:name="android.hardware.microphone" /> |
There was a problem hiding this comment.
Add android:required="false" to avoid filtering devices.
Without required="false", Play Store will filter out devices that lack these hardware features. Most VoIP apps should work on devices without dedicated audio output or microphone hardware (e.g., tablets using Bluetooth).
Proposed fix
- <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" />📝 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.
| <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" /> |
🤖 Prompt for AI Agents
In `@android/app/src/main/AndroidManifest.xml` around lines 25 - 26, Update the
two <uses-feature> entries for android.hardware.audio.output and
android.hardware.microphone so they don't cause Play Store device filtering:
modify the <uses-feature android:name="android.hardware.audio.output" /> and
<uses-feature android:name="android.hardware.microphone" /> elements to include
android:required="false" (i.e., set the required attribute to false for both
features) so devices without those hardware features are not excluded.
| private fun handleVoipIntent(context: Context, intent: Intent): Boolean { | ||
| if (!intent.getBooleanExtra("voipAction", false)) { | ||
| return false | ||
| } | ||
|
|
||
| val notificationId = intent.getIntExtra("notificationId", 0) | ||
| val event = intent.getStringExtra("event") ?: return true | ||
| val callId = intent.getStringExtra("callId") ?: "" | ||
| val callUUID = intent.getStringExtra("callUUID") ?: "" | ||
| val callerName = intent.getStringExtra("callerName") ?: "" | ||
| val host = intent.getStringExtra("host") ?: "" | ||
|
|
||
| Log.d(TAG, "Handling VoIP intent - event: $event, callId: $callId, callUUID: $callUUID") | ||
|
|
||
| VoipNotification.cancelById(context, notificationId) | ||
| VoipModule.storePendingVoipCall(context, callId, callUUID, callerName, host, event) | ||
|
|
||
| // Emit event to JS if app is running | ||
| if (event == "accept") { | ||
| VoipModule.emitCallAnswered(callUUID) | ||
| } | ||
|
|
||
| // Clear the voip flag to prevent re-processing | ||
| intent.removeExtra("voipAction") | ||
|
|
||
| return true |
There was a problem hiding this comment.
Handle malformed VoIP intents without leaving stale state.
If event is missing, the current early return skips cleanup. Also guard notificationId == 0 to avoid canceling an unintended ID.
🛠️ Suggested fix
- val notificationId = intent.getIntExtra("notificationId", 0)
- val event = intent.getStringExtra("event") ?: return true
+ val notificationId = intent.getIntExtra("notificationId", 0)
+ val event = intent.getStringExtra("event")
+ if (event.isNullOrEmpty()) {
+ if (notificationId != 0) {
+ VoipNotification.cancelById(context, notificationId)
+ }
+ intent.removeExtra("voipAction")
+ return true
+ }
@@
- VoipNotification.cancelById(context, notificationId)
+ if (notificationId != 0) {
+ VoipNotification.cancelById(context, notificationId)
+ }📝 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.
| private fun handleVoipIntent(context: Context, intent: Intent): Boolean { | |
| if (!intent.getBooleanExtra("voipAction", false)) { | |
| return false | |
| } | |
| val notificationId = intent.getIntExtra("notificationId", 0) | |
| val event = intent.getStringExtra("event") ?: return true | |
| val callId = intent.getStringExtra("callId") ?: "" | |
| val callUUID = intent.getStringExtra("callUUID") ?: "" | |
| val callerName = intent.getStringExtra("callerName") ?: "" | |
| val host = intent.getStringExtra("host") ?: "" | |
| Log.d(TAG, "Handling VoIP intent - event: $event, callId: $callId, callUUID: $callUUID") | |
| VoipNotification.cancelById(context, notificationId) | |
| VoipModule.storePendingVoipCall(context, callId, callUUID, callerName, host, event) | |
| // Emit event to JS if app is running | |
| if (event == "accept") { | |
| VoipModule.emitCallAnswered(callUUID) | |
| } | |
| // Clear the voip flag to prevent re-processing | |
| intent.removeExtra("voipAction") | |
| return true | |
| private fun handleVoipIntent(context: Context, intent: Intent): Boolean { | |
| if (!intent.getBooleanExtra("voipAction", false)) { | |
| return false | |
| } | |
| val notificationId = intent.getIntExtra("notificationId", 0) | |
| val event = intent.getStringExtra("event") | |
| if (event.isNullOrEmpty()) { | |
| if (notificationId != 0) { | |
| VoipNotification.cancelById(context, notificationId) | |
| } | |
| intent.removeExtra("voipAction") | |
| return true | |
| } | |
| val callId = intent.getStringExtra("callId") ?: "" | |
| val callUUID = intent.getStringExtra("callUUID") ?: "" | |
| val callerName = intent.getStringExtra("callerName") ?: "" | |
| val host = intent.getStringExtra("host") ?: "" | |
| Log.d(TAG, "Handling VoIP intent - event: $event, callId: $callId, callUUID: $callUUID") | |
| if (notificationId != 0) { | |
| VoipNotification.cancelById(context, notificationId) | |
| } | |
| VoipModule.storePendingVoipCall(context, callId, callUUID, callerName, host, event) | |
| // Emit event to JS if app is running | |
| if (event == "accept") { | |
| VoipModule.emitCallAnswered(callUUID) | |
| } | |
| // Clear the voip flag to prevent re-processing | |
| intent.removeExtra("voipAction") | |
| return true | |
| } |
🤖 Prompt for AI Agents
In
`@android/app/src/main/java/chat/rocket/reactnative/notification/NotificationIntentHandler.kt`
around lines 43 - 68, In handleVoipIntent, ensure malformed VoIP intents
(missing event) don't leave stale state: when event is null, still clear the
voip flag and cancel the notification only if notificationId != 0 (avoid
canceling ID 0), but skip calling
VoipModule.storePendingVoipCall/emitCallAnswered; always call
intent.removeExtra("voipAction") before returning and call
VoipNotification.cancelById(context, notificationId) only when notificationId !=
0; keep existing behavior for valid events (store pending call, cancel, emit on
"accept").
| private fun parseVoipPayload(data: Map<String, String>): VoipPayload? { | ||
| val type = data["type"] | ||
| val hasEjson = data.containsKey("ejson") && !data["ejson"].isNullOrEmpty() | ||
|
|
||
| if (type != "incoming_call" || hasEjson) { | ||
| return null | ||
| } | ||
|
|
||
| return try { | ||
| VoipPayload( | ||
| callId = data["callId"], | ||
| calleeId = data["calleeId"], | ||
| caller = data["caller"], | ||
| host = data["host"], | ||
| type = data["type"] | ||
| ) | ||
| } catch (e: Exception) { | ||
| Log.e(TAG, "Failed to parse VoIP payload", e) | ||
| null | ||
| } | ||
| } |
There was a problem hiding this comment.
Validate required VoIP fields before constructing payload.
If any required keys are missing/blank, downstream handling may receive null/empty values and misbehave. Consider early-return guards.
🛠️ Proposed guardrails
- val type = data["type"]
+ val type = data["type"] ?: return null
val hasEjson = data.containsKey("ejson") && !data["ejson"].isNullOrEmpty()
if (type != "incoming_call" || hasEjson) {
return null
}
+ val callId = data["callId"]?.takeIf { it.isNotBlank() } ?: return null
+ val calleeId = data["calleeId"]?.takeIf { it.isNotBlank() } ?: return null
+ val caller = data["caller"]?.takeIf { it.isNotBlank() } ?: return null
+ val host = data["host"]?.takeIf { it.isNotBlank() } ?: return null
+
return try {
VoipPayload(
- callId = data["callId"],
- calleeId = data["calleeId"],
- caller = data["caller"],
- host = data["host"],
- type = data["type"]
+ callId = callId,
+ calleeId = calleeId,
+ caller = caller,
+ host = host,
+ type = type
)
} catch (e: Exception) {
Log.e(TAG, "Failed to parse VoIP payload", e)
null🤖 Prompt for AI Agents
In
`@android/app/src/main/java/chat/rocket/reactnative/notification/RCFirebaseMessagingService.kt`
around lines 65 - 85, parseVoipPayload currently constructs a VoipPayload even
when required fields may be missing; add early-return guards that validate
required keys (at least callId, calleeId, caller, host — type is already
checked) by verifying data contains each key and that none are null or blank
(use isNullOrEmpty) and return null if any are invalid before constructing
VoipPayload; update the parseVoipPayload function to perform these checks so
downstream code never receives a payload with empty critical fields.
| class DeclineReceiver : BroadcastReceiver() { | ||
| override fun onReceive(context: Context, intent: Intent) { | ||
| val callUUID = intent.getStringExtra("callUUID") | ||
| VoipNotification.handleDeclineAction(context, callUUID) | ||
| } | ||
| } |
There was a problem hiding this comment.
DeclineReceiver is not registered in AndroidManifest.xml.
The DeclineReceiver BroadcastReceiver is defined here but not declared in the manifest. The decline action from notifications will not work because Android cannot instantiate an unregistered receiver.
Add to AndroidManifest.xml inside <application>:
<receiver
android:name="chat.rocket.reactnative.notification.VoipNotification$DeclineReceiver"
android:exported="false">
<intent-filter>
<action android:name="chat.rocket.reactnative.ACTION_VOIP_DECLINE" />
</intent-filter>
</receiver>🤖 Prompt for AI Agents
In
`@android/app/src/main/java/chat/rocket/reactnative/notification/VoipNotification.kt`
around lines 81 - 86, The DeclineReceiver BroadcastReceiver defined as class
DeclineReceiver (inside VoipNotification) is never registered in the Android
manifest so decline actions won't be delivered; register it by adding a
<receiver> entry for
chat.rocket.reactnative.notification.VoipNotification$DeclineReceiver inside the
<application> tag of AndroidManifest.xml and include an intent-filter for the
decline action name (the ACTION_VOIP_DECLINE string used by the notification)
and set exported="false" so Android can instantiate the receiver and
VoipNotification.handleDeclineAction(context, callUUID) will be invoked.
| // Get icons | ||
| val packageName = context.packageName | ||
| val smallIconResId = context.resources.getIdentifier("ic_notification", "drawable", packageName) | ||
|
|
There was a problem hiding this comment.
Add fallback for missing notification icon.
Same issue as in VoipForegroundService: if ic_notification is not found, smallIconResId will be 0.
Proposed fix
// Get icons
val packageName = context.packageName
- val smallIconResId = context.resources.getIdentifier("ic_notification", "drawable", packageName)
+ var smallIconResId = context.resources.getIdentifier("ic_notification", "drawable", packageName)
+ if (smallIconResId == 0) {
+ smallIconResId = android.R.drawable.ic_menu_call
+ }🤖 Prompt for AI Agents
In
`@android/app/src/main/java/chat/rocket/reactnative/notification/VoipNotification.kt`
around lines 281 - 284, The code in VoipNotification.kt retrieves
ic_notification via context.resources.getIdentifier and may get 0 if the
drawable is missing; update the smallIcon resolution (where smallIconResId is
computed) to check for 0 and use a safe fallback (e.g., the app's launcher icon
or a platform default like android.R.drawable.sym_def_app_icon or
context.applicationInfo.icon) before passing it to NotificationBuilder so the
notification always has a valid icon; adjust any logic that uses smallIconResId
(smallIconResId variable and the code that sets the notification small icon) to
use the resolved fallback value.
| private fun handleAccept() { | ||
| Log.d(TAG, "Call accepted - callUUID: $callUUID") | ||
| stopRingtone() | ||
|
|
||
| // Cancel notification | ||
| callUUID?.let { uuid -> | ||
| val notificationId = uuid.replace("-", "").hashCode() | ||
| VoipNotification.cancelById(this, notificationId) | ||
| } | ||
|
|
||
| // Store pending call data before emitting event (fixes race condition) | ||
| // if (callUUID != null && callId != null && callerName != null && host != null) { | ||
| // VoipModule.storePendingVoipCall(this, callId, callUUID, callerName, host, "accept") | ||
| // } | ||
|
|
||
| // Launch MainActivity with call data | ||
| val launchIntent = Intent(this, MainActivity::class.java).apply { | ||
| flags = Intent.FLAG_ACTIVITY_NEW_TASK or Intent.FLAG_ACTIVITY_CLEAR_TOP | ||
| putExtra("voipAction", true) | ||
| putExtra("event", "accept") | ||
| putExtra("callId", callId) | ||
| putExtra("callUUID", callUUID) | ||
| putExtra("callerName", callerName) | ||
| putExtra("host", host) | ||
| } | ||
| startActivity(launchIntent) | ||
|
|
||
| finish() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find VoipNotification.kt
file=$(fd -a 'VoipNotification.kt' android/app/src/main/java 2>/dev/null | head -n1)
if [ -z "$file" ]; then
echo "VoipNotification.kt not found"
exit 1
fi
echo "=== File found: $file ==="
echo ""
echo "=== Searching for notificationId, hashCode, replace patterns ==="
rg -n "notificationId|hashCode|replace" "$file" -A 2 -B 2
echo ""
echo "=== Full content of VoipNotification.kt ==="
wc -l "$file"
cat -n "$file"Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 19634
Notification ID mismatch: cancellation will fail
IncomingCallActivity attempts to cancel with callUUID.replace("-", "").hashCode(), but VoipNotification creates the notification using callId.hashCode() (lines 225, 345). Since callUUID is generated separately from callId (via CallIdUUID.generateUUIDv5(callId)), the hash values will not match, causing the cancel operation to silently fail and leave an orphaned notification in the system.
🤖 Prompt for AI Agents
In
`@android/app/src/main/java/chat/rocket/reactnative/voip/IncomingCallActivity.kt`
around lines 130 - 158, In handleAccept(), the notification cancellation uses
callUUID.replace("-", "").hashCode() which won't match the ID used when creating
the notification (VoipNotification uses callId.hashCode()); change the cancel
logic to compute the notificationId from callId (the same way VoipNotification
does) and call VoipNotification.cancelById(this, notificationId) so the IDs
match and the notification is actually removed (reference: handleAccept(),
callUUID, callId, VoipNotification.cancelById).
| private fun createNotification(callerName: String): Notification { | ||
| val packageName = packageName | ||
| val smallIconResId = resources.getIdentifier("ic_notification", "drawable", packageName) | ||
|
|
||
| return NotificationCompat.Builder(this, CHANNEL_ID) | ||
| .setContentTitle("Ongoing call") | ||
| .setContentText("Call with $callerName") | ||
| .setSmallIcon(smallIconResId) | ||
| .setOngoing(true) | ||
| .setPriority(NotificationCompat.PRIORITY_LOW) | ||
| .setCategory(NotificationCompat.CATEGORY_CALL) | ||
| .build() | ||
| } |
There was a problem hiding this comment.
Add fallback for missing notification icon.
If ic_notification drawable is not found, smallIconResId will be 0, which could cause the notification to fail silently or crash on some devices.
Proposed fix
private fun createNotification(callerName: String): Notification {
val packageName = packageName
- val smallIconResId = resources.getIdentifier("ic_notification", "drawable", packageName)
+ var smallIconResId = resources.getIdentifier("ic_notification", "drawable", packageName)
+ if (smallIconResId == 0) {
+ smallIconResId = android.R.drawable.ic_menu_call
+ }
return NotificationCompat.Builder(this, CHANNEL_ID)📝 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.
| private fun createNotification(callerName: String): Notification { | |
| val packageName = packageName | |
| val smallIconResId = resources.getIdentifier("ic_notification", "drawable", packageName) | |
| return NotificationCompat.Builder(this, CHANNEL_ID) | |
| .setContentTitle("Ongoing call") | |
| .setContentText("Call with $callerName") | |
| .setSmallIcon(smallIconResId) | |
| .setOngoing(true) | |
| .setPriority(NotificationCompat.PRIORITY_LOW) | |
| .setCategory(NotificationCompat.CATEGORY_CALL) | |
| .build() | |
| } | |
| private fun createNotification(callerName: String): Notification { | |
| val packageName = packageName | |
| var smallIconResId = resources.getIdentifier("ic_notification", "drawable", packageName) | |
| if (smallIconResId == 0) { | |
| smallIconResId = android.R.drawable.ic_menu_call | |
| } | |
| return NotificationCompat.Builder(this, CHANNEL_ID) | |
| .setContentTitle("Ongoing call") | |
| .setContentText("Call with $callerName") | |
| .setSmallIcon(smallIconResId) | |
| .setOngoing(true) | |
| .setPriority(NotificationCompat.PRIORITY_LOW) | |
| .setCategory(NotificationCompat.CATEGORY_CALL) | |
| .build() | |
| } |
🤖 Prompt for AI Agents
In
`@android/app/src/main/java/chat/rocket/reactnative/voip/VoipForegroundService.kt`
around lines 104 - 116, The createNotification function may pass a zero resource
id when resources.getIdentifier("ic_notification", "drawable", packageName)
doesn't find the drawable; update createNotification to check smallIconResId and
use a safe fallback (e.g., android.R.drawable.sym_def_app_icon or the app's
launcher icon obtained via packageManager) before calling
NotificationCompat.Builder(this, CHANNEL_ID).setSmallIcon(...), ensuring
setSmallIcon always receives a valid non-zero drawable id (refer to
smallIconResId, createNotification, CHANNEL_ID, and NotificationCompat.Builder
to locate the change).
| <?xml version="1.0" encoding="utf-8"?> | ||
| <LinearLayout xmlns:android="http://schemas.android.com/apk/res/android" | ||
| android:layout_width="match_parent" | ||
| android:layout_height="match_parent" | ||
| android:orientation="vertical" | ||
| android:gravity="center" | ||
| android:background="#000000" | ||
| android:padding="32dp"> | ||
|
|
||
| <!-- Caller Avatar --> | ||
| <ImageView | ||
| android:id="@+id/caller_avatar" | ||
| android:layout_width="120dp" | ||
| android:layout_height="120dp" | ||
| android:layout_marginBottom="24dp" | ||
| android:src="@mipmap/ic_launcher" | ||
| android:contentDescription="Caller avatar" | ||
| android:scaleType="centerCrop" | ||
| android:background="@android:drawable/dialog_holo_light_frame" /> | ||
|
|
||
| <!-- Caller Name --> | ||
| <TextView | ||
| android:id="@+id/caller_name" | ||
| android:layout_width="wrap_content" | ||
| android:layout_height="wrap_content" | ||
| android:layout_marginBottom="8dp" | ||
| android:text="Unknown" | ||
| android:textColor="#FFFFFF" | ||
| android:textSize="28sp" | ||
| android:textStyle="bold" /> | ||
|
|
||
| <!-- Incoming Call Label --> | ||
| <TextView | ||
| android:id="@+id/incoming_label" | ||
| android:layout_width="wrap_content" | ||
| android:layout_height="wrap_content" | ||
| android:layout_marginBottom="48dp" | ||
| android:text="Incoming call" | ||
| android:textColor="#CCCCCC" | ||
| android:textSize="16sp" /> | ||
|
|
||
| <!-- Action Buttons --> | ||
| <LinearLayout | ||
| android:layout_width="match_parent" | ||
| android:layout_height="wrap_content" | ||
| android:orientation="horizontal" | ||
| android:gravity="center" | ||
| android:layout_marginTop="32dp"> | ||
|
|
||
| <!-- Decline Button --> | ||
| <ImageButton | ||
| android:id="@+id/btn_decline" | ||
| android:layout_width="80dp" | ||
| android:layout_height="80dp" | ||
| android:layout_marginEnd="32dp" | ||
| android:background="@android:drawable/ic_menu_close_clear_cancel" | ||
| android:backgroundTint="#F44336" | ||
| android:contentDescription="Decline call" | ||
| android:scaleType="centerInside" /> | ||
|
|
||
| <!-- Accept Button --> | ||
| <ImageButton | ||
| android:id="@+id/btn_accept" | ||
| android:layout_width="80dp" | ||
| android:layout_height="80dp" | ||
| android:background="@android:drawable/ic_menu_call" | ||
| android:backgroundTint="#4CAF50" | ||
| android:contentDescription="Accept call" | ||
| android:scaleType="centerInside" /> |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Android best practices hardcoded strings colors resources localization
💡 Result:
Best practices (Android): avoid hardcoded strings/colors; support resources + localization
Strings & localization
- Never hardcode UI text in code or XML layouts—put all user-visible text in
res/values/strings.xmlso it can be extracted and translated. Include complete default strings (fallback) inres/values/strings.xmleven if you ship only some translations. [1] - Use string resources with formatting (e.g.,
%1$s,%d) instead of concatenation, and use plurals viagetQuantityString()/<plurals>for grammatically-correct quantities across languages. [5] - Don’t hardcode date/number/currency formats—use locale-aware formatters (Android 7.0+/API 24+ and up) to avoid incorrect formats in different locales. [2]
- Keep culture/language-specific resources in locale-qualified folders (e.g.,
values-es,values-en-rGB). [3]
Colors, themes, and dark mode
- Avoid hardcoded color literals like
#RRGGBBin layouts/styles; prefer theme attributes (semantic colors) so your UI adapts across themes (including night mode). [6] - Define your app’s colors via themes + styles (e.g.,
colorPrimary, etc.) and reference them through the theme rather than direct hex values. [7]
Tooling / enforcement
- Enable/heed Lint checks for hardcoded values (e.g., hardcoded colors) to catch issues early in CI and code review. [4]
Sources: Android Developers docs on localization, multilingual support/formatting, string resources/plurals, and theming. [1][2][3][5][7] plus Android guidance on preferring theme attributes and avoiding hardcoded colors. [4][6]
🏁 Script executed:
# Check if the file exists and review its content
find . -name "activity_incoming_call.xml" -type fRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 138
🏁 Script executed:
cat -n ./android/app/src/main/res/layout/activity_incoming_call.xmlRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 3106
Move hardcoded strings and colors into resource files.
All user-visible text and colors are currently hardcoded, which blocks localization and theming. Move text to res/values/strings.xml and colors to res/values/colors.xml, then reference them via @string/ and @color/ attributes.
Example changes
- android:background="#000000"
+ android:background="@color/voip_incoming_background"
@@
- android:contentDescription="Caller avatar"
+ android:contentDescription="@string/voip_caller_avatar"
@@
- android:text="Unknown"
- android:textColor="#FFFFFF"
+ android:text="@string/voip_unknown_caller"
+ android:textColor="@color/voip_text_primary"
@@
- android:text="Incoming call"
- android:textColor="#CCCCCC"
+ android:text="@string/voip_incoming_call"
+ android:textColor="@color/voip_text_secondary"
@@
- android:backgroundTint="#F44336"
- android:contentDescription="Decline call"
+ android:backgroundTint="@color/voip_decline"
+ android:contentDescription="@string/voip_decline_call"
@@
- android:backgroundTint="#4CAF50"
- android:contentDescription="Accept call"
+ android:backgroundTint="@color/voip_accept"
+ android:contentDescription="@string/voip_accept_call"🤖 Prompt for AI Agents
In `@android/app/src/main/res/layout/activity_incoming_call.xml` around lines 1 -
69, Replace hardcoded user-visible strings and color hexes in this layout with
resource references: move texts ("Unknown", "Incoming call", "Decline call",
"Accept call", avatar contentDescription) into res/values/strings.xml and
replace android:text and android:contentDescription on TextViews and
ImageButtons (ids: caller_name, incoming_label, btn_decline, btn_accept,
caller_avatar) with `@string/`...; move colors (`#000000`, `#FFFFFF`, `#CCCCCC`,
`#F44336`, `#4CAF50`) into res/values/colors.xml and replace android:background,
android:textColor, android:backgroundTint attributes in the root LinearLayout,
caller_name, incoming_label, btn_decline and btn_accept with `@color/`...; ensure
the new resource names are descriptive and update all attribute references in
activity_incoming_call.xml accordingly.
| import type { TurboModule } from 'react-native'; | ||
| import { TurboModuleRegistry } from 'react-native'; | ||
|
|
||
| export interface Spec extends TurboModule { | ||
| getPendingVoipCall(): Promise<string | null>; | ||
| clearPendingVoipCall(): void; | ||
| } | ||
|
|
||
| export default TurboModuleRegistry.getEnforcing<Spec>('VoipModule'); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for VoipModule in iOS directory
find . -type d -name "ios" | head -5
rg -n "VoipModule" ios -g '*.{m,mm,swift}' 2>/dev/null || echo "No matches or ios dir not found"Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 306
🏁 Script executed:
#!/bin/bash
# Search for imports of NativeVoipAndroid
rg -n "NativeVoipAndroid" --type ts --type tsxRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 105
🏁 Script executed:
#!/bin/bash
# Check if there's a corresponding iOS file or platform-specific pattern
fd "VoipAndroid\|VoipIOS\|Voip" app/lib/native -g "*.ts"
# Also check how other TurboModules are handled
fd "Module.*\\.ts" app/lib/native | head -10Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 128
🏁 Script executed:
#!/bin/bash
# Check if Platform is used elsewhere in the codebase for conditional exports
rg -n "Platform\.OS.*android" app/lib --type ts -A 2 -B 1 | head -30Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 2445
🏁 Script executed:
#!/bin/bash
# Search for how NativeVoipAndroid is imported and used
rg -n "NativeVoipAndroid\|from.*native.*Voip" app/lib --type ts -A 2 -B 1Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 60
🏁 Script executed:
#!/bin/bash
# Check if there's a corresponding iOS file
find app/lib -name "*Voip*" -type fRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 113
🏁 Script executed:
#!/bin/bash
# Check the voip service initialization
cat -n app/lib/services/voip/getInitialEvents.ts | head -50Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 2356
🏁 Script executed:
#!/bin/bash
# Check full getInitialEvents.ts for error handling around the import
cat -n app/lib/services/voip/getInitialEvents.ts | tail -80Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 2514
🏁 Script executed:
#!/bin/bash
# Search for error handling or try-catch in voip initialization
rg -n "try|catch|NativeVoipModule" app/lib/services/voip/getInitialEvents.ts -A 1 -B 1Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 1389
🏁 Script executed:
#!/bin/bash
# Check if there's a wrapper around NativeVoipAndroid export in that file
cat -n app/lib/native/NativeVoipAndroid.tsRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 436
🏁 Script executed:
#!/bin/bash
# Search for how other NativeAndroid modules handle this (like NativePushNotificationAndroid)
cat -n app/lib/native/NativePushNotificationAndroid.tsRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 447
🏁 Script executed:
#!/bin/bash
# Check the actual import pattern and error handling in notifications/index.ts
sed -n '98,110p' app/lib/notifications/index.tsRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 673
Use a null-safe module lookup to prevent crashes on iOS.
getEnforcing('VoipModule') throws at import time if the module isn't registered (e.g., iOS). This module is imported unconditionally in app/lib/services/voip/getInitialEvents.ts line 7, so the error occurs before platform checks can execute. Switch to TurboModuleRegistry.get() instead, which returns null for unregistered modules—similar to NativePushNotificationAndroid.ts. This allows the existing if (!NativeVoipModule) guard at line 171 of getInitialEvents.ts to work correctly.
🛠️ Suggested fix
-export default TurboModuleRegistry.getEnforcing<Spec>('VoipModule');
+export default TurboModuleRegistry.get<Spec>('VoipModule');📝 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.
| import type { TurboModule } from 'react-native'; | |
| import { TurboModuleRegistry } from 'react-native'; | |
| export interface Spec extends TurboModule { | |
| getPendingVoipCall(): Promise<string | null>; | |
| clearPendingVoipCall(): void; | |
| } | |
| export default TurboModuleRegistry.getEnforcing<Spec>('VoipModule'); | |
| import type { TurboModule } from 'react-native'; | |
| import { TurboModuleRegistry } from 'react-native'; | |
| export interface Spec extends TurboModule { | |
| getPendingVoipCall(): Promise<string | null>; | |
| clearPendingVoipCall(): void; | |
| } | |
| export default TurboModuleRegistry.get<Spec>('VoipModule'); |
🤖 Prompt for AI Agents
In `@app/lib/native/NativeVoipAndroid.ts` around lines 1 - 9, The current module
lookup uses TurboModuleRegistry.getEnforcing('VoipModule') which throws if the
native module isn't registered; change it to
TurboModuleRegistry.get<Spec>('VoipModule') so the lookup returns null on iOS
instead of crashing—update the default export in NativeVoipAndroid.ts to use
get(...) (referenced Spec and TurboModuleRegistry) so the existing null check in
getInitialEvents.ts (guarding NativeVoipModule) works as intended.
| if (token) { | ||
| deviceToken = token; | ||
| console.log('[push.ts] Registered for push notifications:', token); | ||
| } |
There was a problem hiding this comment.
Avoid logging push tokens in production logs.
Device tokens are sensitive identifiers; logging them can leak PII and undermine security. Prefer removing this log or guarding it behind a dev-only flag.
🔧 Suggested change (dev-only logging)
if (token) {
deviceToken = token;
- console.log('[push.ts] Registered for push notifications:', token);
+ if (__DEV__) {
+ console.log('[push.ts] Registered for push notifications:', token);
+ }
}📝 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 (token) { | |
| deviceToken = token; | |
| console.log('[push.ts] Registered for push notifications:', token); | |
| } | |
| if (token) { | |
| deviceToken = token; | |
| if (__DEV__) { | |
| console.log('[push.ts] Registered for push notifications:', token); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@app/lib/notifications/push.ts` around lines 186 - 189, The console.log in
push.ts is printing sensitive device tokens; remove the log or guard it behind a
development-only check. Specifically, update the code around the
deviceToken/token assignment (where console.log('[push.ts] Registered for push
notifications:', token) is called) to either delete that console.log entirely or
wrap it with a dev-only condition (e.g., NODE_ENV === 'development' or a feature
flag) or use a debug-level logger that is disabled in production so deviceToken
and token are never written to production logs.
Proposed changes
Issue(s)
https://rocketchat.atlassian.net/browse/VMUX-6
How to test or reproduce
Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.