feat: Voice support #6918
feat: Voice support #6918
Conversation
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis pull request introduces comprehensive VoIP calling capabilities to the React Native Rocket.Chat app, including native module integrations for iOS and Android, a complete call UI, state management via Zustand, WebRTC media session orchestration, push notification handling, and deep linking support for incoming calls across both platforms. Changes
Sequence DiagramssequenceDiagram
actor User
participant PushService as Push Service
participant NativeModule as Native Module
participant AppDelegate as iOS AppDelegate<br/>or FCM Service
participant CallKeep as RNCallKeep
participant ReactApp as React App
participant UI as Call UI
User->>PushService: Incoming VoIP Call
PushService->>AppDelegate: PushKit Notification
AppDelegate->>NativeModule: didReceiveIncomingPush()
NativeModule->>NativeModule: Parse callId, caller
NativeModule->>NativeModule: Generate UUID v5
NativeModule->>CallKeep: displayIncomingCall()
CallKeep->>ReactApp: Emit VoipCallAccepted
ReactApp->>ReactApp: dispatch voipCallOpen()
ReactApp->>ReactApp: navigate to CallView
ReactApp->>UI: Render IncomingCall/CallView
UI->>User: Display caller info & actions
sequenceDiagram
participant UI as Call UI
participant Store as useCallStore
participant Media as MediaSessionInstance
participant WebRTC as WebRTC Processor
participant Signaling as Signal Transport
participant Server as Remote Peer
UI->>Store: User presses Accept/Answer
Store->>Store: updateFromCall()
Store->>Media: setCall(call, callUUID)
Media->>WebRTC: Create session with ICE servers
WebRTC->>WebRTC: Gather ICE candidates
WebRTC->>Signaling: Send SDP offer
Signaling->>Server: Transmit offer
Server->>Signaling: Return answer SDP
Signaling->>WebRTC: Receive answer
WebRTC->>WebRTC: Connect peer
WebRTC->>Media: emit 'stateChange' (active)
Media->>Store: Update callState to 'active'
Store->>UI: Call connected, enable actions
UI->>User: Show call duration, mute/hold/speaker
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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.
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); }
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/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; };
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
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
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").
android/app/src/main/java/chat/rocket/reactnative/notification/RCFirebaseMessagingService.kt
Outdated
Show resolved
Hide resolved
| 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.
| 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.
… structure, removing deprecated modules and consolidating functionality. Update imports in MainApplication and NotificationIntentHandler to reflect changes. This cleanup enhances code organization and prepares for future VoIP feature enhancements.
…ed and declined, switch from SharedPreferences to in-memory storage for pending VoIP call data, and update method signatures for better clarity. This cleanup enhances performance and prepares for future VoIP feature improvements.
…ate call data, streamline notification processing, and enhance method signatures across the VoIP module. This update improves code clarity and prepares for future feature enhancements.
…kage to the main package and removing the obsolete NativeVoipSpec class. Adjust imports in MainApplication and VoipModule to reflect these changes, enhancing code organization and maintainability.
diegolmello
force-pushed
the
feat.voip-lib-new
branch
from
7348135 to
3914562
Compare
diegolmello
requested a deployment
to
approve_e2e_testing
GitHub Actions
Waiting
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
android/app/src/main/java/chat/rocket/reactnative/notification/NotificationIntentHandler.kt (1)
76-77:⚠️ Potential issue | 🟡 Minor
eventmissing still silently skips cleanup.When
eventisnull(line 77), the earlyreturn trueskips notification cancellation. IfnotificationIdis valid, this leaves an orphaned notification in the system tray.Proposed fix
- val event = intent.getStringExtra("event") ?: return true + val event = intent.getStringExtra("event") + if (event.isNullOrEmpty()) { + if (notificationId != 0) { + VideoConfNotification.cancelById(context, notificationId) + } + intent.removeExtra("videoConfAction") + return true + }
🤖 Fix all issues with AI agents
In
`@android/app/src/main/java/chat/rocket/reactnative/notification/NotificationIntentHandler.kt`:
- Around line 46-64: In handleVoipIntent, when
intent.getBooleanExtra("voipAction", false) is true but voipPayload is null or
not an incoming call, consume the intent and clear the voip flag instead of
returning false; specifically, in the branch that currently returns false for
(voipPayload == null || !voipPayload.isVoipIncomingCall()), call
intent.removeExtra("voipAction") (and do not attempt VoipNotification.cancelById
or VoipModule.storePendingVoipCall since payload is invalid) and then return
true so malformed VoIP intents are not routed to other handlers.
In `@android/app/src/main/java/chat/rocket/reactnative/voip/VoipModule.kt`:
- Around line 17-72: The companion's shared state (pendingVoipCallData,
pendingVoipCallTimestamp) is accessed from multiple threads causing visibility
and TOCTOU issues; mark these fields `@Volatile` and protect compound operations
by synchronizing the read-then-clear sequence: make storePendingVoipCall and
clearPendingVoipCallInternal synchronized (or annotate with `@Synchronized`) and
wrap the logic in getPendingVoipCall that reads timestamp, validates age,
converts to WritableMap and clears in a single synchronized block (or use an
AtomicReference for pendingVoipCallData with an atomic get-and-set) so access
from VoipNotification/FCM thread and JS thread is safe while keeping
emitVoipCallEvent unchanged.
In `@android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt`:
- Around line 253-263: Replace hardcoded English strings in the
NotificationCompat.Builder block with localized string resources: use
context.getString(R.string.incoming_call) for setContentTitle,
context.getString(R.string.call_from, caller) or a formatted resource for
setContentText, and context.getString(R.string.decline) /
context.getString(R.string.accept) for the addAction labels; update the builder
calls (setContentTitle, setContentText, addAction) accordingly and add the
corresponding entries (incoming_call, call_from with %s placeholder, decline,
accept) to strings.xml for each locale.
- Around line 144-148: The check in VoipNotification.kt uses isNullOrEmpty() on
non-nullable String parameters (callUUID and caller) and logs only callUUID;
change the conditional to check for emptiness (use isEmpty() or isBlank() on
callUUID and caller) and update the Log.e(TAG, ...) message to mention both
callUUID and caller so it accurately reflects which parameter is empty; locate
the check around the register-with-TelecomManager logic that references
callUUID, caller and Log.e to apply this fix.
- Around line 217-222: The accept PendingIntent currently created by
acceptIntent (used with createPendingIntent and notificationId + 1) lacks a
distinguishing action so MainActivity can't tell an "accept" launch from a
normal app start; set an explicit action (e.g., ACTION_ACCEPT) on acceptIntent
before creating the PendingIntent and ensure the constant name matches the
decline path (ACTION_DECLINE) so MainActivity can check intent.action and route
the accept flow accordingly while still including voipPayload extras.
- Around line 168-178: The incoming-call extras are constructing a tel: URI for
a display-name VoIP caller; update the Uri creation in VoipNotification (the
callerUri variable built with Uri.fromParts) to use PhoneAccount.SCHEME_SIP
instead of PhoneAccount.SCHEME_TEL so the Telecom framework treats the
identifier as a SIP/VoIP handle; ensure the modified callerUri is still placed
into TelecomManager.EXTRA_INCOMING_CALL_ADDRESS and that other extras
(EXTRA_CALL_UUID, EXTRA_CALLER_NAME, callUUID, name, handle) remain unchanged.
In `@android/app/src/main/java/chat/rocket/reactnative/voip/VoipPayload.kt`:
- Around line 55-63: The VoipPayload.fromMap function currently logs the entire
incoming payload map via Log.d which may expose PII in production; update the
function to stop logging the full `data` map—either remove the Log.d call
entirely or gate it behind a debug-only check (e.g., BuildConfig.DEBUG) and, if
any logging is needed, log only non-sensitive fields or masked values (use the
`fromMap` method and fields `callId`, `caller`, `host`, `type` to build a safe
log message instead of the full map).
In `@android/app/src/main/java/chat/rocket/reactnative/voip/VoipTurboPackage.kt`:
- Line 1: The package declaration in VoipTurboPackage.kt is incorrect for its
directory; change the package line to "package chat.rocket.reactnative.voip" in
VoipTurboPackage.kt and then update any imports/usages (e.g., in
MainApplication.kt) to import chat.rocket.reactnative.voip.VoipTurboPackage
instead of chat.rocket.reactnative.VoipTurboPackage so the class resolution
matches the file path.
In `@app/lib/services/voip/getInitialEvents.ts`:
- Around line 89-92: The forEach callback on subscriptions is using an
implicit-return arrow (sub => sub.remove()) which triggers a lint warning;
change the callback in the cleanup return function to a block-body arrow (e.g.,
subscriptions.forEach(sub => { sub.remove(); });) so the callback does not
implicitly return the result of sub.remove(), updating the cleanup returned by
getInitialEvents (the function that returns () => { ... }) to use this
block-style callback for subscriptions.forEach.
- Around line 49-64: The VoipCallAccepted listener is marked async though it
doesn't use await; remove the unnecessary async modifier on the handler passed
to voipModuleEmitter.addListener('VoipCallAccepted', ...) to satisfy ESLint.
Locate the handler function that calls NativeVoipModule.clearPendingVoipCall,
useCallStore.getState().setCallUUID and store.dispatch(voipCallOpen(...)) and
change its signature to a plain synchronous function (no async) while leaving
the try/catch and internal calls unchanged.
In `@ios/AppDelegate.swift`:
- Around line 139-142: The call to reportNewIncomingCall is currently passing
withCompletionHandler: nil which swallows CallKit errors; change the call site
(the reportNewIncomingCall invocation that includes fromPushKit: true and
payload: payload.dictionaryPayload) to provide a completion handler that checks
the provided Error? and logs it (e.g., via NSLog or your app logger) so failures
(Do Not Disturb, config issues, etc.) are surfaced rather than ignored.
In `@ios/Libraries/VoipModule.mm`:
- Around line 102-108: The overrides for addListener: and removeListeners: are
currently empty and must forward to the superclass so RCTEventEmitter can manage
its listener count and call startObserving(); update the VoipModule.mm
implementations of addListener:(NSString *)eventName and
removeListeners:(double)count to call the corresponding super implementations
(e.g., [super addListener:eventName]; and [super removeListeners:count];) so
_hasListeners transitions correctly and queued events in _delayedEvents are
delivered to JS.
In `@ios/Libraries/VoipService.swift`:
- Around line 168-170: The notificationId property uses callId.hashValue which
is process-randomized; replace that with a deterministic string-to-int hash
(e.g., implement a stable 32-bit hash such as DJB2 or FNV-1a over callId) and
return that value from the `@objc` public var notificationId instead of
callId.hashValue; update references to notificationId accordingly to ensure
parity with Android's stable hash approach and keep the result within Int (and
non-negative if needed) before returning.
- Around line 98-114: Update the notification name used when posting the VoIP
push event in storePendingVoipCall: change the NSNotification.Name string from
"VoipCallAccepted" to "VoipCallReceived" so the event semantics match
didReceiveIncomingPush behavior; update any corresponding observers/listeners
that watch NSNotification.Name("VoipCallAccepted") to listen for
NSNotification.Name("VoipCallReceived") instead (search for usages of
"VoipCallAccepted" and replace with "VoipCallReceived", ensuring
storePendingVoipCall, pendingVoipCallData handling, and any ObjC++ bridge
observers are updated).
In `@ios/RocketChatRN.xcodeproj/project.pbxproj`:
- Line 1564: The shellScript for the RocketChatRN target's "Bundle React Native
code and images" step contains a hardcoded NODE_BINARY pointing to a developer's
nvm path; change it to use a generic node invocation or environment variable
like NODE_BINARY=node (matching the Rocket.Chat target) so CI and other
developers won't break—update the shellScript entry for the RocketChatRN target
(the shellScript string shown) to export NODE_BINARY=node instead of the
absolute /Users/diegomello/... path.
In `@package.json`:
- Around line 217-223: Update native references to match the new codegenConfig
name and package: replace old SSLPinning identifiers and package imports with
the new ones from codegenConfig ("RocketChatSpecs" and
"chat.rocket.reactnative"). Concretely, in Android update MainApplication.kt
imports/registration to use chat.rocket.reactnative.SSLPinningTurboPackage (or
the new generated package/class name) instead of
chat.rocket.reactnative.networking.SSLPinningTurboPackage, and change the
module-name check in MainApplicationModuleProvider.cpp from "SSLPinning" to
"RocketChatSpecs". On iOS update AppDelegate.swift and any JS usage of
SSLPinning() to the new generated API name (e.g., RocketChatSpecs() or the
generated spec initializer), and change the Java spec constant
NativeSSLPinningSpec.NAME to "RocketChatSpecs" (or rename the spec class to
match the new codegen output). Ensure all imports, registration calls, and
literal module-name strings consistently use the new package
"chat.rocket.reactnative" and the new spec name "RocketChatSpecs".
🧹 Nitpick comments (14)
app/views/CallView/components/CallerInfo.tsx (2)
10-10: Remove commented-out code before merging.The commented-out
Statusimport (Line 10) and its usage (Line 30) leave dead code in the component. Line 29 renders an emptyViewwith a background color but no visible content — this produces a small colored badge with nothing inside it, which seems unintentional.If the Status indicator is planned for a follow-up, consider removing it entirely and tracking it as a TODO in your issue tracker instead.
Also applies to: 29-31
25-54: Consider adding accessibility labels for VoIP caller info.For a calling screen that may be used in lock-screen/heads-up contexts, screen reader support is important. The
Textelements lackaccessibilityRoleand the outer container has noaccessibilityLabelsummarizing the caller. At minimum, addingaccessibilityRole="header"to the caller name text and anaccessibilityLabelon the container would improve VoiceOver/TalkBack support.android/app/src/main/java/chat/rocket/reactnative/utils/CallIdUUIDModule.kt (1)
15-23: Namespace comment is slightly misleading.The comment says "Fixed namespace UUID for VoIP calls" but this is actually the standard RFC 4122 URL namespace, repurposed here for call ID hashing. It works correctly for deterministic UUID generation, but a custom namespace UUID would be more semantically appropriate for application-specific identifiers like VoIP call IDs. Not a functional issue as long as both platforms use the same namespace.
package.json (2)
208-208:react-domversion mismatch withreact.
reactis at19.0.0(Line 88) butreact-domin devDependencies is still at18.2.0. While this is a devDependency (likely for Storybook), mixing React 18 and 19 can cause subtle issues in Storybook rendering and type mismatches. This was likely a pre-existing issue, but worth aligning when convenient.
50-50: Committing the.tgzfile works but breaks the established pattern for@rocket.chatdependencies.The bundled tarball at
./packages/rocket.chat-media-signaling-0.1.0.tgzresolves the previous CI failure caused by the absolute local path. However, this approach deviates from how other@rocket.chatpackages are referenced in this file—@rocket.chat/mobile-cryptoand@rocket.chat/sdkuse GitHub references (RocketChat/rocket.chat-mobile-crypto,RocketChat/Rocket.Chat.js.SDK#mobile).Committing binary archives to the repository increases repo size and makes dependency updates opaque on version bumps. Consider publishing this package to a GitHub repository and using a GitHub reference (e.g.,
"RocketChat/media-signaling#v0.1.0") to align with the established pattern.android/app/src/main/java/chat/rocket/reactnative/voip/IncomingCallActivity.kt (2)
134-144: Decline path doesn't actually reject the call on the server.The TODO on line 141 is important — without calling the server REST API to decline the call, the caller will not know the call was rejected and the server-side call state will be left hanging. This could result in repeated ring notifications or the caller waiting indefinitely.
Would you like me to open an issue to track implementing the server-side call rejection?
146-153:onBackPressed()is deprecated on API 33+.Consider using
OnBackPressedDispatcherwithOnBackPressedCallbackfor forward compatibility.This is low priority for a proof-of-concept, but worth noting for production readiness.
android/app/src/main/java/chat/rocket/reactnative/voip/VoipPayload.kt (1)
66-74:fromBundleis more lenient thanfromMap, which could allow partially invalid payloads.
fromBundleonly requirescallIdand defaultscaller,host, andtypeto empty strings, whilefromMaprequires all four fields and checkstype == "incoming_call". A VoipPayload created viafromBundlewith an emptytypewill pass construction but failisVoipIncomingCall().This is fine if callers always validate with
isVoipIncomingCall()afterfromBundle, whichIncomingCallActivityandNotificationIntentHandlerdo. Just flagging for awareness — consider adding a comment documenting this design choice.app/lib/services/voip/getInitialEvents.ts (1)
172-201: AndroidgetInitialEventsAndroidclears pending call before validating it.On line 180,
clearPendingVoipCall()is called before the validation on line 182. IfcallIdorhostare missing, the pending call data is already lost and cannot be retried or inspected. Consider moving the clear after validation, consistent with the iOS path (line 157):Proposed fix
const getInitialEventsAndroid = (): boolean => { try { const pendingCall = NativeVoipModule.getPendingVoipCall() as VoipPayload; if (!pendingCall) { console.log('[VoIP][Android] No pending VoIP call'); return false; } - NativeVoipModule.clearPendingVoipCall(); - if (!pendingCall.callId || !pendingCall.host) { console.log('[VoIP][Android] Missing required call data'); + NativeVoipModule.clearPendingVoipCall(); return false; } useCallStore.getState().setCallUUID(pendingCall.callUUID); store.dispatch( voipCallOpen({ callId: pendingCall.callId, callUUID: pendingCall.callUUID, host: pendingCall.host }) ); + NativeVoipModule.clearPendingVoipCall(); return true;ios/Libraries/VoipService.swift (1)
172-174:callUUIDis recomputed on every access.Unlike Android's
VoipPayload.ktwherecallUUIDis aval(computed once at construction), this Swift computed property invokesCallIdUUID.generateUUIDv5(from:)on every access, including SHA-1 hashing. Consider making it a lazy stored property:Proposed fix
- `@objc` public var callUUID: String { - return CallIdUUID.generateUUIDv5(from: callId) - } + `@objc` public lazy var callUUID: String = CallIdUUID.generateUUIDv5(from: callId)Note: This requires changing VoipPayload from
let-style immutability to supportlazy, and the class already usesclass(notstruct), so this works.android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt (1)
249-267: Dead code:avatarBitmapis alwaysnull.
avatarBitmapis unconditionally set tonullon Line 250, making theif (avatarBitmap != null)block on Lines 265-267 unreachable. Either remove the dead branch or implement avatar loading.android/app/src/main/java/chat/rocket/reactnative/notification/RCFirebaseMessagingService.kt (1)
7-7:parseEjsonandGsoninstance are unused dead code.The
parseEjsonmethod (Lines 60-71) is never called in this file, and thegsoncompanion field (Line 22) exists only to serve it. Either remove both or wireparseEjsoninto the notification flow if it's intended for use.Proposed removal
import com.google.firebase.messaging.FirebaseMessagingService import com.google.firebase.messaging.RemoteMessage -import com.google.gson.Gson import chat.rocket.reactnative.voip.VoipNotification import chat.rocket.reactnative.voip.VoipPayloadcompanion object { private const val TAG = "RocketChat.FCM" - private val gson = Gson() }- /** - * Safely parses ejson string to Ejson object. - */ - private fun parseEjson(ejsonStr: String?): Ejson? { - if (ejsonStr.isNullOrEmpty() || ejsonStr == "{}") { - return null - } - - return try { - gson.fromJson(ejsonStr, Ejson::class.java) - } catch (e: Exception) { - Log.e(TAG, "Failed to parse ejson", e) - null - } - }Also applies to: 22-22, 57-71
app/lib/native/NativeVoip.ts (1)
17-17: Consider using a more specific return type thanObject.
getPendingVoipCall(): Object | nullloses type information at the JS boundary. If TurboModule codegen supports it, using a typed shape (e.g.,{ callId: string; caller: string; ... } | null) or at least{[key: string]: string} | nullwould give consumers compile-time safety and reduce the need for runtime validation.android/app/src/main/java/chat/rocket/reactnative/voip/VoipModule.kt (1)
62-71: Unnecessary try/catch inclearPendingVoipCallInternal.Setting fields to
nulland0cannot throw. The try/catch adds noise. The iOS counterpart (VoipService.swift:clearPendingVoipCallInternal) performs the same operation without exception handling.Proposed simplification
`@JvmStatic` fun clearPendingVoipCallInternal() { - try { - pendingVoipCallData = null - pendingVoipCallTimestamp = 0 - Log.d(TAG, "Cleared pending VoIP call data") - } catch (e: Exception) { - Log.e(TAG, "Error clearing pending VoIP call", e) - } + pendingVoipCallData = null + pendingVoipCallTimestamp = 0 + Log.d(TAG, "Cleared pending VoIP call data") }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
ios/Podfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (24)
android/app/src/main/java/chat/rocket/reactnative/MainApplication.ktandroid/app/src/main/java/chat/rocket/reactnative/notification/NotificationIntentHandler.ktandroid/app/src/main/java/chat/rocket/reactnative/notification/RCFirebaseMessagingService.ktandroid/app/src/main/java/chat/rocket/reactnative/utils/CallIdUUIDModule.ktandroid/app/src/main/java/chat/rocket/reactnative/utils/CallIdUUIDTurboPackage.ktandroid/app/src/main/java/chat/rocket/reactnative/utils/NativeCallIdUUIDSpec.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/IncomingCallActivity.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/VoipModule.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/VoipPayload.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/VoipTurboPackage.ktapp/containers/CallHeader/components/Title.tsxapp/definitions/Voip.tsapp/lib/native/NativeVoip.tsapp/lib/services/restApi.tsapp/lib/services/voip/getInitialEvents.tsapp/views/CallView/components/CallerInfo.tsxapp/views/CallView/styles.tsios/AppDelegate.swiftios/Libraries/VoipModule.mmios/Libraries/VoipService.swiftios/RocketChatRN-Bridging-Header.hios/RocketChatRN.xcodeproj/project.pbxprojpackage.json
🚧 Files skipped from review as they are similar to previous changes (5)
- app/views/CallView/styles.ts
- app/containers/CallHeader/components/Title.tsx
- app/definitions/Voip.ts
- app/lib/services/restApi.ts
- android/app/src/main/java/chat/rocket/reactnative/MainApplication.kt
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6930
File: package.json:101-101
Timestamp: 2026-02-05T13:55:06.688Z
Learning: The RocketChat/Rocket.Chat.ReactNative repository uses a fork of react-native-image-crop-picker (RocketChat/react-native-image-crop-picker) with custom Android edge-to-edge fixes, not the upstream ivpusic/react-native-image-crop-picker package. Dependencies should reference commit pins from the RocketChat fork.
Learnt from: OtavioStasiak
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6499
File: app/containers/ServerItem/index.tsx:34-36
Timestamp: 2025-12-17T15:56:22.578Z
Learning: In the Rocket.Chat React Native codebase, for radio button components on iOS, include the selection state ("Selected"/"Unselected") in the accessibilityLabel instead of using accessibilityState={{ checked: hasCheck }}, because iOS VoiceOver has known issues with accessibilityRole="radio" + accessibilityState that prevent correct state announcement.
📚 Learning: 2026-02-05T13:55:00.974Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6930
File: package.json:101-101
Timestamp: 2026-02-05T13:55:00.974Z
Learning: In this repository, the dependency on react-native-image-crop-picker should reference the RocketChat fork (RocketChat/react-native-image-crop-picker) with explicit commit pins, not the upstream ivpusic/react-native-image-crop-picker. Update package.json dependencies (and any lockfile) to point to the fork URL and a specific commit, ensuring edge-to-edge Android fixes are included. This pattern should apply to all package.json files in the repo that declare this dependency.
Applied to files:
package.json
📚 Learning: 2025-12-17T15:56:22.578Z
Learnt from: OtavioStasiak
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6499
File: app/containers/ServerItem/index.tsx:34-36
Timestamp: 2025-12-17T15:56:22.578Z
Learning: In the Rocket.Chat React Native codebase, for radio button components on iOS, include the selection state ("Selected"/"Unselected") in the accessibilityLabel instead of using accessibilityState={{ checked: hasCheck }}, because iOS VoiceOver has known issues with accessibilityRole="radio" + accessibilityState that prevent correct state announcement.
Applied to files:
package.json
🧬 Code graph analysis (6)
ios/AppDelegate.swift (2)
ios/Libraries/VoipService.swift (3)
voipRegistration(31-51)didUpdatePushCredentials(54-79)didReceiveIncomingPush(82-96)ios/Libraries/CallIdUUID.swift (1)
generateUUIDv5(40-66)
app/views/CallView/components/CallerInfo.tsx (4)
app/theme.tsx (1)
useTheme(29-29)app/lib/services/voip/useCallStore.ts (1)
useCallContact(189-189)app/views/CallView/styles.ts (1)
styles(5-74)app/containers/message/MessageAvatar.tsx (1)
AvatarContainer(13-17)
app/lib/services/voip/getInitialEvents.ts (5)
app/lib/services/voip/pushTokenAux.ts (1)
setVoipPushToken(6-8)app/definitions/Voip.ts (1)
VoipPayload(7-13)app/lib/services/voip/useCallStore.ts (1)
useCallStore(57-185)app/actions/deepLinking.ts (1)
voipCallOpen(47-52)app/lib/services/voip/MediaSessionInstance.ts (1)
mediaSessionInstance(199-199)
ios/Libraries/VoipService.swift (2)
app/definitions/Voip.ts (1)
VoipPayload(7-13)android/app/src/main/java/chat/rocket/reactnative/voip/VoipPayload.kt (1)
callId(10-76)
android/app/src/main/java/chat/rocket/reactnative/voip/VoipModule.kt (1)
ios/Libraries/VoipService.swift (1)
clearPendingVoipCallInternal(143-149)
android/app/src/main/java/chat/rocket/reactnative/voip/VoipPayload.kt (1)
app/definitions/Voip.ts (1)
VoipPayload(7-13)
🪛 Biome (2.3.13)
app/lib/services/voip/getInitialEvents.ts
[error] 91-91: This callback passed to forEach() iterable method should not return a value.
Either remove this return or remove the returned value.
(lint/suspicious/useIterableCallbackReturn)
🪛 Clang (14.0.6)
ios/RocketChatRN-Bridging-Header.h
[error] 5-5: 'RNCallKeep/RNCallKeep.h' file not found
(clang-diagnostic-error)
🪛 ESLint
app/lib/services/voip/getInitialEvents.ts
[error] 49-49: Async arrow function has no 'await' expression.
(require-await)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: format
- GitHub Check: ESLint and Test / run-eslint-and-test
🔇 Additional comments (17)
app/views/CallView/components/CallerInfo.tsx (2)
4-4: No changes needed. The import at line 4 correctly resolves toAvatarContainerfromapp/containers/Avatar, which is exported fromAvatarContainer.tsxand properly accepts thetext,size,borderRadius, andchildrenprops used in the component.
17-23: No guard needed—contactis always an object.
useCallContact()returnsstate.contact, which is initialized as an empty object{}(not null or undefined). TheCallContactinterface defines all properties as optional, so accessingcontact.displayNameon an empty object returnsundefined, not a TypeError. The code already handles this gracefully with the||operator. The current implementation is safe without optional chaining.Likely an incorrect or invalid review comment.
android/app/src/main/java/chat/rocket/reactnative/utils/CallIdUUIDTurboPackage.kt (1)
1-30: LGTM!Standard TurboModule package registration. The metadata flags are appropriate — no eager init needed for a stateless utility module.
android/app/src/main/java/chat/rocket/reactnative/utils/CallIdUUIDModule.kt (2)
67-72: LGTM!Clean TurboModule bridge —
getName()returns the spec constant andtoUUIDdelegates directly to the statelessCallIdUUID.generateUUIDv5.
33-60: UUID v5 implementation is correct and cross-platform parity is verified.Both Android (Kotlin) and iOS (Swift) implementations use the same namespace UUID (
6ba7b811-9dad-11d1-80b4-00c04fd430c8) and identical algorithms: concatenate namespace + callId UTF-8 bytes, SHA-1 hash, set version bits to 5 (0x50), set variant bits to 10 (0x80), and format as UUID. The implementations will produce identical output for the same input. The method is stateless and safe to call from synchronous bridges.android/app/src/main/java/chat/rocket/reactnative/utils/NativeCallIdUUIDSpec.kt (1)
1-19: LGTM!The spec is well-structured. Using
isBlockingSynchronousMethod = trueis appropriate here since UUID generation is a fast, CPU-bound operation and the JS caller likely needs the result synchronously. The hand-written spec cleanly defines the contract forCallIdUUIDModule.Minor note:
getName()is already implemented here (Line 15), so the override inCallIdUUIDModule(Line 69 ofCallIdUUIDModule.kt) is redundant — though being explicit is fine too.ios/RocketChatRN.xcodeproj/project.pbxproj (3)
294-308: VoIP native module files correctly added to both main app targets.
CallIdUUID.swift/.mandVoipService.swift/VoipModule.mmare consistently added to bothRocketChatRNandRocket.Chatsource build phases, and appropriately excluded fromNotificationServiceand Watch targets.
1661-1680: New[CP] Embed Pods Frameworksphases correctly embed WebRTC and Hermes.Both the
RocketChatRNandRocket.Chattargets now embedWebRTC.framework(from JitsiWebRTC) andhermes.framework, which is consistent with the VoIP feature requirements.
1143-1161: Clean reorganization of native modules into aLibrariesgroup.All native bridge files (SecureStorage, A11yFlowModule, MMKVKeyManager, SSLPinning) are now co-located with the new VoIP files (CallIdUUID, VoipModule, VoipService) under a single
Librariesgroup.ios/RocketChatRN-Bridging-Header.h (1)
5-5: Thereact-native-callkeepdependency is properly configured and installed.The dependency is declared in
package.jsonat version 4.3.16, the Podfile uses auto-linking viause_native_modules!to resolve it, andPodfile.lockconfirms RNCallKeep is installed. The import#import <RNCallKeep/RNCallKeep.h>in the bridging header is correct and will be available oncepod installis run.package.json (1)
125-125: [email protected] is compatible with React Native 0.79. The library's peer dependency supportsreact-native >= 0.60.0, and version 124.0.6 explicitly added Android compatibility for RN 0.80+. As a patch release, 124.0.7 does not introduce breaking changes and is safe to use with RN 0.79.app/lib/services/voip/getInitialEvents.ts (1)
100-167: iOS initial event flow looks solid.The implementation correctly:
- Validates pending call data before processing
- Matches CallKeep answer events with UUID case-insensitive comparison (line 135)
- Clears both native pending data and CallKeep initial events after processing
- Has proper error boundaries with try/catch
ios/Libraries/VoipModule.mm (1)
39-61: Delayed event flush pattern is well-designed.The buffering of events in
_delayedEventswhen no JS listeners are active, followed by a flush instartObserving, correctly handles the timing gap between native event emission and JS listener registration.android/app/src/main/java/chat/rocket/reactnative/notification/RCFirebaseMessagingService.kt (1)
41-46: VoIP routing looks correct — early detection and return prevents falling through to regular notification handling.The
VoipPayload.fromMap(data)check cleanly separates VoIP payloads from regular push notifications.app/lib/native/NativeVoip.ts (1)
1-37: TurboModule spec follows standard conventions — LGTM.Method signatures, event emitter boilerplate (
addListener/removeListeners), andgetEnforcingusage all align with React Native TurboModule patterns.android/app/src/main/java/chat/rocket/reactnative/voip/VoipModule.kt (2)
83-95:getPendingVoipCallcorrectly implements 5-minute expiry and clear-after-read — logic is sound.The expiry check and clear-after-retrieval pattern matches the documented contract in the TurboModule spec. Thread-safety concern is addressed in the companion object comment above.
108-111:registerVoipTokenno-op on Android is appropriate.Android uses FCM for push notifications, and the docstring in
NativeVoip.tsconfirms this is the expected behavior.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| private fun handleVoipIntent(context: Context, intent: Intent): Boolean { | ||
| if (!intent.getBooleanExtra("voipAction", false)) { | ||
| return false | ||
| } | ||
| val voipPayload = VoipPayload.fromBundle(intent.extras) | ||
| if (voipPayload == null || !voipPayload.isVoipIncomingCall()) { | ||
| return false | ||
| } | ||
|
|
||
| Log.d(TAG, "Handling VoIP intent - voipPayload: $voipPayload") | ||
|
|
||
| VoipNotification.cancelById(context, voipPayload.notificationId) | ||
| VoipModule.storePendingVoipCall(voipPayload) | ||
|
|
||
| // Clear the voip flag to prevent re-processing | ||
| intent.removeExtra("voipAction") | ||
|
|
||
| return true | ||
| } |
There was a problem hiding this comment.
Malformed VoIP intents with voipAction=true fall through to unrelated handlers.
If voipAction is true but the payload is invalid (lines 51-53 return false), the intent is not consumed and falls through to video-conference and regular notification handling. While those handlers are unlikely to match, the voipAction extra is also never cleared in this path.
Consider returning true (consumed) when voipAction is set but the payload is invalid, to prevent the intent from being misrouted:
Proposed fix
private fun handleVoipIntent(context: Context, intent: Intent): Boolean {
if (!intent.getBooleanExtra("voipAction", false)) {
return false
}
val voipPayload = VoipPayload.fromBundle(intent.extras)
if (voipPayload == null || !voipPayload.isVoipIncomingCall()) {
+ intent.removeExtra("voipAction")
- return false
+ return true
}🤖 Prompt for AI Agents
In
`@android/app/src/main/java/chat/rocket/reactnative/notification/NotificationIntentHandler.kt`
around lines 46 - 64, In handleVoipIntent, when
intent.getBooleanExtra("voipAction", false) is true but voipPayload is null or
not an incoming call, consume the intent and clear the voip flag instead of
returning false; specifically, in the branch that currently returns false for
(voipPayload == null || !voipPayload.isVoipIncomingCall()), call
intent.removeExtra("voipAction") (and do not attempt VoipNotification.cancelById
or VoipModule.storePendingVoipCall since payload is invalid) and then return
true so malformed VoIP intents are not routed to other handlers.
| companion object { | ||
| private const val TAG = "RocketChat.VoipModule" | ||
| private const val EVENT_VOIP_CALL = "VoipCallAccepted" | ||
|
|
||
| private var reactContextRef: WeakReference<ReactApplicationContext>? = null | ||
| private var pendingVoipCallData: VoipPayload? = null | ||
| private var pendingVoipCallTimestamp: Long = 0 | ||
|
|
||
| /** | ||
| * Sets the React context reference for event emission. | ||
| */ | ||
| @JvmStatic | ||
| fun setReactContext(context: ReactApplicationContext) { | ||
| reactContextRef = WeakReference(context) | ||
| } | ||
|
|
||
| /** | ||
| * Emits a VoIP call event to JavaScript when the app is running. | ||
| */ | ||
| @JvmStatic | ||
| fun emitVoipCallEvent(voipPayload: VoipPayload) { | ||
| try { | ||
| reactContextRef?.get()?.let { context -> | ||
| if (context.hasActiveReactInstance()) { | ||
| context | ||
| .getJSModule(DeviceEventManagerModule.RCTDeviceEventEmitter::class.java) | ||
| .emit(EVENT_VOIP_CALL, voipPayload.toWritableMap()) | ||
| } | ||
| } | ||
| } catch (e: Exception) { | ||
| Log.e(TAG, "Failed to emit VoIP call event", e) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Stores VoIP call data for JS to retrieve. | ||
| * Also emits an event if the app is running. | ||
| */ | ||
| @JvmStatic | ||
| fun storePendingVoipCall(voipPayload: VoipPayload) { | ||
| pendingVoipCallData = voipPayload | ||
| pendingVoipCallTimestamp = System.currentTimeMillis() | ||
| emitVoipCallEvent(voipPayload) | ||
| } | ||
|
|
||
| @JvmStatic | ||
| fun clearPendingVoipCallInternal() { | ||
| try { | ||
| pendingVoipCallData = null | ||
| pendingVoipCallTimestamp = 0 | ||
| Log.d(TAG, "Cleared pending VoIP call data") | ||
| } catch (e: Exception) { | ||
| Log.e(TAG, "Error clearing pending VoIP call", e) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Thread-safety concern: companion object state is accessed from multiple threads without synchronization.
pendingVoipCallData and pendingVoipCallTimestamp are written by storePendingVoipCall (called from the FCM background thread via VoipNotification) and read by getPendingVoipCall/clearPendingVoipCall (called from the JS thread). Without synchronization, you risk torn reads, stale visibility, and TOCTOU between the null check in getPendingVoipCall (Line 84) and the actual use (Line 91).
At minimum, mark the fields @Volatile; for full correctness, use a synchronized block or an AtomicReference.
Proposed fix using `@Volatile`
- private var pendingVoipCallData: VoipPayload? = null
- private var pendingVoipCallTimestamp: Long = 0
+ `@Volatile`
+ private var pendingVoipCallData: VoipPayload? = null
+ `@Volatile`
+ private var pendingVoipCallTimestamp: Long = 0For stronger guarantees (preventing TOCTOU in getPendingVoipCall), wrap the read-then-clear in a synchronized block:
`@JvmStatic`
`@Synchronized`
fun storePendingVoipCall(voipPayload: VoipPayload) { ... }
// and in the instance:
override fun getPendingVoipCall(): WritableMap? {
synchronized(Companion) {
val data = pendingVoipCallData ?: return null
if (System.currentTimeMillis() - pendingVoipCallTimestamp > 5 * 60 * 1000) {
clearPendingVoipCallInternal()
return null
}
val result = data.toWritableMap()
clearPendingVoipCallInternal()
return result
}
}🤖 Prompt for AI Agents
In `@android/app/src/main/java/chat/rocket/reactnative/voip/VoipModule.kt` around
lines 17 - 72, The companion's shared state (pendingVoipCallData,
pendingVoipCallTimestamp) is accessed from multiple threads causing visibility
and TOCTOU issues; mark these fields `@Volatile` and protect compound operations
by synchronizing the read-then-clear sequence: make storePendingVoipCall and
clearPendingVoipCallInternal synchronized (or annotate with `@Synchronized`) and
wrap the logic in getPendingVoipCall that reads timestamp, validates age,
converts to WritableMap and clears in a single synchronized block (or use an
AtomicReference for pendingVoipCallData with an atomic get-and-set) so access
from VoipNotification/FCM thread and JS thread is safe while keeping
emitVoipCallEvent unchanged.
| // Validate inputs | ||
| if (callUUID.isNullOrEmpty() || caller.isNullOrEmpty()) { | ||
| Log.e(TAG, "Cannot register call with TelecomManager: callUUID is null or empty") | ||
| return | ||
| } |
There was a problem hiding this comment.
isNullOrEmpty() on non-nullable String parameters; misleading error message.
callUUID and caller are typed as String (non-null in Kotlin), so isNullOrEmpty() will never be null — only empty. More importantly, the error message only mentions callUUID but the check also covers caller. Use isEmpty() and fix the log message.
Proposed fix
- if (callUUID.isNullOrEmpty() || caller.isNullOrEmpty()) {
- Log.e(TAG, "Cannot register call with TelecomManager: callUUID is null or empty")
+ if (callUUID.isEmpty() || caller.isEmpty()) {
+ Log.e(TAG, "Cannot register call with TelecomManager: callUUID or caller is empty")
return
}🤖 Prompt for AI Agents
In `@android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt`
around lines 144 - 148, The check in VoipNotification.kt uses isNullOrEmpty() on
non-nullable String parameters (callUUID and caller) and logs only callUUID;
change the conditional to check for emptiness (use isEmpty() or isBlank() on
callUUID and caller) and update the Log.e(TAG, ...) message to mention both
callUUID and caller so it accurately reflects which parameter is empty; locate
the check around the register-with-TelecomManager logic that references
callUUID, caller and Log.e to apply this fix.
| // Create extras for the incoming call | ||
| val extras = Bundle().apply { | ||
| val callerUri = Uri.fromParts(PhoneAccount.SCHEME_TEL, caller, null) | ||
| putParcelable(TelecomManager.EXTRA_INCOMING_CALL_ADDRESS, callerUri) | ||
| putString("EXTRA_CALL_UUID", callUUID) | ||
| putString("EXTRA_CALLER_NAME", caller) | ||
| // Legacy keys for backward compatibility | ||
| putString("callUUID", callUUID) | ||
| putString("name", caller) | ||
| putString("handle", caller) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "SCHEME_TEL|SCHEME_SIP|EXTRA_INCOMING_CALL_ADDRESS" --type kotlin --type javaRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 405
🏁 Script executed:
# Also check the full context of the VoipNotification.kt file to understand the caller parameter
fd "VoipNotification.kt" -t fRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 152
🏁 Script executed:
# Search for how caller parameter is defined and used
rg -n "fun.*VoipNotification|caller.*=" android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt --context 5Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 1828
🏁 Script executed:
# Find the VoipPayload class definition
fd "VoipPayload" -t fRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 147
🏁 Script executed:
# Search for VoipPayload data class definition
rg -n "data class VoipPayload|class VoipPayload" --type kotlin --type java -A 10Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 1105
🏁 Script executed:
# Check how caller is set/used in the codebase
rg -n "voipPayload\.caller|\.caller\s*=" --type kotlin --type java -B 2 -A 2Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 1828
🏁 Script executed:
# Check for any test data or examples showing what caller contains
rg -n "caller.*=|caller.*:" android/app/src/main/java/chat/rocket/reactnative/voip/ --type kotlin --type java -B 1 -A 1 | head -40Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 3890
🏁 Script executed:
# Check package.json for react-native-callkeep version and any usage documentation
cat android/app/build.gradle | grep -i callkeepRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 60
🏁 Script executed:
# Look for any CallKeep or TelecomManager usage patterns in the codebase
rg -n "setHandle|setAddress|EXTRA_INCOMING_CALL_ADDRESS" --type kotlin --type java -B 2 -A 2Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 794
🏁 Script executed:
# Look for any other CallKeep or TelecomManager usage patterns for reference
rg -n "CallKeepModule|addCall|CallKitAddCall" --type kotlin --type java -B 2 -A 5Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 60
🏁 Script executed:
# Check if there's any mention of SCHEME_SIP or alternative schemes elsewhere
rg -n "SCHEME|Uri\.fromParts" --type kotlin --type javaRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 241
🌐 Web query:
Android Telecom EXTRA_INCOMING_CALL_ADDRESS SCHEME_TEL SCHEME_SIP VoIP username
💡 Result:
TelecomManager.EXTRA_INCOMING_CALL_ADDRESS is the Bundle extra your ConnectionService supplies when reporting a new incoming call via TelecomManager.addNewIncomingCall(...). Its value is a Uri “handle” identifying the caller. (developer.android.com)
Which scheme to use (SCHEME_TEL vs SCHEME_SIP)
Android Telecom officially recognizes:
PhoneAccount.SCHEME_TEL="tel"(phone-number style handles)PhoneAccount.SCHEME_SIP="sip"(SIP/VoIP-style handles) (developer.android.com)
For a VoIP username (not an E.164 phone number), you generally want SCHEME_SIP and put your username in the SIP URI scheme-specific part.
Example: incoming call address for a VoIP username
val extras = Bundle().apply {
putParcelable(
TelecomManager.EXTRA_INCOMING_CALL_ADDRESS,
Uri.fromParts(PhoneAccount.SCHEME_SIP, "[email protected]", null)
// produces: sip:[email protected]
)
}
telecomManager.addNewIncomingCall(phoneAccountHandle, extras)Uri.fromParts("sip", "[email protected]", null) is the typical way to represent “VoIP username@domain” as sip:[email protected]. (developer.android.com)
Also set display name (important for non-number IDs)
Because many VoIP identifiers aren’t phone numbers, Android specifically recommends also setting:
Connection.setAddress(Uri, presentation)Connection.setCallerDisplayName(name, presentation)(developer.android.com)
Practical note
Some platform features (e.g., logging for self-managed calls) may only work when the call “handle” scheme is tel or sip, so stick to those for maximum compatibility. (developer.android.com)
Citations:
- 1: https://developer.android.com/reference/kotlin/android/telecom/TelecomManager?utm_source=openai
- 2: https://developer.android.com/reference/android/telecom/PhoneAccount?utm_source=openai
- 3: https://developer.android.com/reference/android/telecom/PhoneAccount?utm_source=openai
- 4: https://developer.android.com/reference/android/telecom/ConnectionService?utm_source=openai
- 5: https://developer.android.com/reference/android/telecom/PhoneAccount?utm_source=openai
Use PhoneAccount.SCHEME_SIP instead of SCHEME_TEL for VoIP caller identifiers.
The caller parameter is a display name (used for EXTRA_CALLER_NAME and displayed in the UI), not a phone number. Uri.fromParts(PhoneAccount.SCHEME_TEL, caller, null) incorrectly wraps it in a tel: URI scheme, which the Android Telecom framework expects only for phone-number-style handles. For VoIP identifiers, PhoneAccount.SCHEME_SIP is the correct scheme. Replace line 170 with:
val callerUri = Uri.fromParts(PhoneAccount.SCHEME_SIP, caller, null)🤖 Prompt for AI Agents
In `@android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt`
around lines 168 - 178, The incoming-call extras are constructing a tel: URI for
a display-name VoIP caller; update the Uri creation in VoipNotification (the
callerUri variable built with Uri.fromParts) to use PhoneAccount.SCHEME_SIP
instead of PhoneAccount.SCHEME_TEL so the Telecom framework treats the
identifier as a SIP/VoIP handle; ensure the modified callerUri is still placed
into TelecomManager.EXTRA_INCOMING_CALL_ADDRESS and that other extras
(EXTRA_CALL_UUID, EXTRA_CALLER_NAME, callUUID, name, handle) remain unchanged.
| // Create Accept action | ||
| val acceptIntent = Intent(context, MainActivity::class.java).apply { | ||
| flags = Intent.FLAG_ACTIVITY_NEW_TASK or Intent.FLAG_ACTIVITY_CLEAR_TOP or Intent.FLAG_ACTIVITY_SINGLE_TOP | ||
| putExtras(voipPayload.toBundle()) | ||
| } | ||
| val acceptPendingIntent = createPendingIntent(notificationId + 1, acceptIntent) |
There was a problem hiding this comment.
Accept intent lacks a distinguishing action — MainActivity cannot differentiate an accept from a normal app launch.
The accept PendingIntent targets MainActivity with extras but no explicit action. If MainActivity checks intent.action to route incoming calls, this will be indistinguishable from a launcher intent. The decline intent correctly sets action = ACTION_DECLINE; consider adding action = ACTION_ACCEPT here as well.
Proposed fix
val acceptIntent = Intent(context, MainActivity::class.java).apply {
+ action = ACTION_ACCEPT
flags = Intent.FLAG_ACTIVITY_NEW_TASK or Intent.FLAG_ACTIVITY_CLEAR_TOP or Intent.FLAG_ACTIVITY_SINGLE_TOP
putExtras(voipPayload.toBundle())
}🤖 Prompt for AI Agents
In `@android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt`
around lines 217 - 222, The accept PendingIntent currently created by
acceptIntent (used with createPendingIntent and notificationId + 1) lacks a
distinguishing action so MainActivity can't tell an "accept" launch from a
normal app start; set an explicit action (e.g., ACTION_ACCEPT) on acceptIntent
before creating the PendingIntent and ensure the constant name matches the
decline path (ACTION_DECLINE) so MainActivity can check intent.action and route
the accept flow accordingly while still including voipPayload extras.
| public final class VoipService: NSObject { | ||
|
|
||
| // MARK: - Constants | ||
|
|
||
| private static let TAG = "RocketChat.VoipModule" | ||
|
|
||
| // MARK: - Static Properties | ||
|
|
||
| private static var pendingVoipCallData: VoipPayload? | ||
| private static var pendingVoipCallTimestamp: TimeInterval = 0 | ||
| private static var isVoipRegistered = false | ||
| private static var lastVoipToken: String = "" | ||
| private static var voipRegistry: PKPushRegistry? |
There was a problem hiding this comment.
Static mutable state accessed from multiple threads without synchronization.
pendingVoipCallData, pendingVoipCallTimestamp, and other static properties are written from the main thread (PushKit callbacks via storePendingVoipCall) and read from the JS thread (via getPendingVoipCall). This is a potential data race.
Consider adding a serial dispatch queue or using @MainActor to ensure safe access:
Possible approach
private static let queue = DispatchQueue(label: "chat.rocket.voip.service")
public static func storePendingVoipCall(_ payload: VoipPayload) {
queue.sync {
pendingVoipCallData = payload
pendingVoipCallTimestamp = Date().timeIntervalSince1970
}
// Post notification outside the queue
NotificationCenter.default.post(...)
}
public static func getPendingVoipCall() -> [String: Any]? {
return queue.sync { /* existing logic */ }
}
ios/Libraries/VoipService.swift
Outdated
| /// Stores VoIP call data for JS to retrieve. | ||
| @objc | ||
| public static func storePendingVoipCall(_ payload: VoipPayload) { | ||
| pendingVoipCallData = payload | ||
| pendingVoipCallTimestamp = Date().timeIntervalSince1970 | ||
|
|
||
| #if DEBUG | ||
| print("[\(TAG)] Stored pending VoIP call: \(payload.callId)") | ||
| #endif | ||
|
|
||
| // Emit event via notification (the ObjC++ bridge observes this) | ||
| NotificationCenter.default.post( | ||
| name: NSNotification.Name("VoipCallAccepted"), | ||
| object: nil, | ||
| userInfo: payload.toDictionary() | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "VoipCallAccepted" -C3Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 4028
🏁 Script executed:
rg -n "VoipCallReceived" -C3Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 60
🏁 Script executed:
# Check the didReceiveIncomingPush method to understand the call flow
rg -n "didReceiveIncomingPush" -C5Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 2606
🏁 Script executed:
# Look for how the pending VoIP call data is consumed on the JS side
rg -n "pendingVoipCallData\|voipCallOpen" -C3Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 60
🏁 Script executed:
# View the full implementation of didReceiveIncomingPush to see if it calls storePendingVoipCall
sed -n '81,105p' ios/Libraries/VoipService.swiftRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 989
🏁 Script executed:
# Also check if there are any other callers of storePendingVoipCall
rg -n "storePendingVoipCall" -C2Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 1975
🏁 Script executed:
# Check what happens in the VoipCallAccepted listener in JS
sed -n '49,66p' app/lib/services/voip/getInitialEvents.tsRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 615
🏁 Script executed:
# Search for voipCallOpen to see if it's called elsewhere
rg -n "voipCallOpen" -C3Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 3663
Rename "VoipCallAccepted" event to "VoipCallReceived" — it fires when push is received, not when user accepts.
The event is posted from storePendingVoipCall (called by didReceiveIncomingPush when a VoIP push arrives), triggering the JS side to immediately dispatch voipCallOpen before the user accepts. The current name suggests user acceptance, creating semantic confusion. Rename to "VoipCallReceived" to clarify that this event fires automatically when the push arrives while the app is active, not when the user answers.
🤖 Prompt for AI Agents
In `@ios/Libraries/VoipService.swift` around lines 98 - 114, Update the
notification name used when posting the VoIP push event in storePendingVoipCall:
change the NSNotification.Name string from "VoipCallAccepted" to
"VoipCallReceived" so the event semantics match didReceiveIncomingPush behavior;
update any corresponding observers/listeners that watch
NSNotification.Name("VoipCallAccepted") to listen for
NSNotification.Name("VoipCallReceived") instead (search for usages of
"VoipCallAccepted" and replace with "VoipCallReceived", ensuring
storePendingVoipCall, pendingVoipCallData handling, and any ObjC++ bridge
observers are updated).
| @objc public var notificationId: Int { | ||
| return callId.hashValue | ||
| } |
There was a problem hiding this comment.
hashValue is not stable across process launches in Swift.
Swift's String.hashValue is randomized per process execution (since Swift 4.2). On Android, String.hashCode() is deterministic and stable. This means notificationId will differ between app launches on iOS, making it unreliable if ever used to cancel or match notifications across process boundaries.
If notificationId is only used within a single process lifetime on iOS (e.g., not persisted), this is fine. But for cross-platform parity and robustness, consider using a stable hash (e.g., a simple DJB2 or the same approach as the Android side):
Proposed fix (stable hash)
`@objc` public var notificationId: Int {
- return callId.hashValue
+ // Use a stable hash consistent with Android's String.hashCode()
+ var hash: Int32 = 0
+ for char in callId.unicodeScalars {
+ hash = 31 &* hash &+ Int32(truncatingIfNeeded: char.value)
+ }
+ return Int(hash)
}📝 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.
| @objc public var notificationId: Int { | |
| return callId.hashValue | |
| } | |
| `@objc` public var notificationId: Int { | |
| // Use a stable hash consistent with Android's String.hashCode() | |
| var hash: Int32 = 0 | |
| for char in callId.unicodeScalars { | |
| hash = 31 &* hash &+ Int32(truncatingIfNeeded: char.value) | |
| } | |
| return Int(hash) | |
| } |
🤖 Prompt for AI Agents
In `@ios/Libraries/VoipService.swift` around lines 168 - 170, The notificationId
property uses callId.hashValue which is process-randomized; replace that with a
deterministic string-to-int hash (e.g., implement a stable 32-bit hash such as
DJB2 or FNV-1a over callId) and return that value from the `@objc` public var
notificationId instead of callId.hashValue; update references to notificationId
accordingly to ensure parity with Android's stable hash approach and keep the
result within Int (and non-negative if needed) before returning.
| runOnlyForDeploymentPostprocessing = 0; | ||
| shellPath = /bin/sh; | ||
| shellScript = "export EXTRA_PACKAGER_ARGS=\"--sourcemap-output $TMPDIR/$(md5 -qs \"$CONFIGURATION_BUILD_DIR\")-main.jsbundle.map\"\nexport NODE_BINARY=node\n../node_modules/react-native/scripts/react-native-xcode.sh\n"; | ||
| shellScript = "export EXTRA_PACKAGER_ARGS=\"--sourcemap-output $TMPDIR/$(md5 -qs \"$CONFIGURATION_BUILD_DIR\")-main.jsbundle.map\"\nexport NODE_BINARY=/Users/diegomello/.nvm/versions/node/v22.14.0/bin/node\n../node_modules/react-native/scripts/react-native-xcode.sh\n"; |
There was a problem hiding this comment.
Hardcoded developer-specific NODE_BINARY path will break CI and other developers' builds.
The RocketChatRN target's "Bundle React Native code and images" script uses an absolute path to a specific developer's nvm installation:
/Users/diegomello/.nvm/versions/node/v22.14.0/bin/node
The Rocket.Chat target (Line 1851) correctly uses export NODE_BINARY=node. This should be consistent.
Proposed fix
- shellScript = "export EXTRA_PACKAGER_ARGS=\"--sourcemap-output $TMPDIR/$(md5 -qs \"$CONFIGURATION_BUILD_DIR\")-main.jsbundle.map\"\nexport NODE_BINARY=/Users/diegomello/.nvm/versions/node/v22.14.0/bin/node\n../node_modules/react-native/scripts/react-native-xcode.sh\n";
+ shellScript = "export EXTRA_PACKAGER_ARGS=\"--sourcemap-output $TMPDIR/$(md5 -qs \"$CONFIGURATION_BUILD_DIR\")-main.jsbundle.map\"\nexport NODE_BINARY=node\n../node_modules/react-native/scripts/react-native-xcode.sh\n";📝 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.
| shellScript = "export EXTRA_PACKAGER_ARGS=\"--sourcemap-output $TMPDIR/$(md5 -qs \"$CONFIGURATION_BUILD_DIR\")-main.jsbundle.map\"\nexport NODE_BINARY=/Users/diegomello/.nvm/versions/node/v22.14.0/bin/node\n../node_modules/react-native/scripts/react-native-xcode.sh\n"; | |
| shellScript = "export EXTRA_PACKAGER_ARGS=\"--sourcemap-output $TMPDIR/$(md5 -qs \"$CONFIGURATION_BUILD_DIR\")-main.jsbundle.map\"\nexport NODE_BINARY=node\n../node_modules/react-native/scripts/react-native-xcode.sh\n"; |
🤖 Prompt for AI Agents
In `@ios/RocketChatRN.xcodeproj/project.pbxproj` at line 1564, The shellScript for
the RocketChatRN target's "Bundle React Native code and images" step contains a
hardcoded NODE_BINARY pointing to a developer's nvm path; change it to use a
generic node invocation or environment variable like NODE_BINARY=node (matching
the Rocket.Chat target) so CI and other developers won't break—update the
shellScript entry for the RocketChatRN target (the shellScript string shown) to
export NODE_BINARY=node instead of the absolute /Users/diegomello/... path.
| "codegenConfig": { | ||
| "name": "SSLPinning", | ||
| "name": "RocketChatSpecs", | ||
| "type": "modules", | ||
| "jsSrcsDir": "./app/lib/native", | ||
| "android": { | ||
| "javaPackageName": "chat.rocket.reactnative.networking" | ||
| "javaPackageName": "chat.rocket.reactnative" | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for any remaining references to the old codegen config name and package name
echo "=== Searching for old codegen name 'SSLPinning' ==="
rg -n 'SSLPinning' --type-add 'native:*.java,*.kt,*.m,*.mm,*.swift,*.h,*.podspec,*.gradle' -g '!node_modules' 2>&1 || echo "No results found"
echo ""
echo "=== Searching for old package name 'chat.rocket.reactnative.networking' ==="
rg -n 'chat\.rocket\.reactnative\.networking' -g '!node_modules' 2>&1 || echo "No results found"
echo ""
echo "=== Checking package.json structure around codegen config ==="
jq '.codegenConfig' package.json 2>/dev/null || cat package.json | sed -n '215,225p'Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 9046
Native module integration broken by incomplete codegenConfig refactoring.
The codegen configuration was renamed from SSLPinning / chat.rocket.reactnative.networking to RocketChatSpecs / chat.rocket.reactnative, but the native module implementations were not updated to match. This creates a critical mismatch:
- Android:
MainApplication.kt(line 18, 44) still imports and registersSSLPinningTurboPackagefrom the oldchat.rocket.reactnative.networkingpackage, while codegen now targetschat.rocket.reactnative - Android:
MainApplicationModuleProvider.cppstill checks for module name"SSLPinning", but codegen now generates specs underRocketChatSpecs - iOS:
AppDelegate.swiftand JavaScript code still referenceSSLPinning()directly - Java spec class
NativeSSLPinningSpec.NAMEis still hardcoded to"SSLPinning"
The codegen will generate new spec files in the new package, but all native code will continue looking for modules in the old locations under the old names. This will cause module registration failures at runtime.
Either revert the codegen config rename and use separate configs for each module (SSL pinning, VoIP), or complete the refactoring by updating all native module registrations, spec classes, and module name references to use the new configuration.
🤖 Prompt for AI Agents
In `@package.json` around lines 217 - 223, Update native references to match the
new codegenConfig name and package: replace old SSLPinning identifiers and
package imports with the new ones from codegenConfig ("RocketChatSpecs" and
"chat.rocket.reactnative"). Concretely, in Android update MainApplication.kt
imports/registration to use chat.rocket.reactnative.SSLPinningTurboPackage (or
the new generated package/class name) instead of
chat.rocket.reactnative.networking.SSLPinningTurboPackage, and change the
module-name check in MainApplicationModuleProvider.cpp from "SSLPinning" to
"RocketChatSpecs". On iOS update AppDelegate.swift and any JS usage of
SSLPinning() to the new generated API name (e.g., RocketChatSpecs() or the
generated spec initializer), and change the Java spec constant
NativeSSLPinningSpec.NAME to "RocketChatSpecs" (or rename the spec class to
match the new codegen output). Ensure all imports, registration calls, and
literal module-name strings consistently use the new package
"chat.rocket.reactnative" and the new spec name "RocketChatSpecs".
1 participant
Proposed changes
Issue(s)
https://rocketchat.atlassian.net/browse/VMUX-6
https://rocketchat.atlassian.net/browse/VMUX-17
https://rocketchat.atlassian.net/browse/VMUX-36
How to test or reproduce
Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit
New Features
Tests
Chores