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
|
![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.
| 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.
| "@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.
| "react-native-animatable": "1.3.3", | ||
| "react-native-background-timer": "2.4.1", | ||
| "react-native-bootsplash": "^6.3.8", | ||
| "react-native-callkeep": "^4.3.16", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
react-native-callkeep 4.3.16 compatibility React Native 0.79
💡 Result:
Short answer: react-native-callkeep 4.3.16 can be used with React Native 0.79, but there are reported compatibility issues — several GitHub issues show runtime/behaviour problems on Android and iOS with RN 0.79 (and with the New Architecture). Test carefully and watch the repo issues for fixes. [1][2][3]
Sources:
- GitHub issue reporting Android behavior with RN 0.79 / callkeep 4.3.16. [1]
- GitHub issue reporting iOS "No such module / New Architecture" problems with callkeep 4.3.16. [2]
- npm / Expo config-plugin listing that ties react-native-callkeep 4.3.16 to recent Expo / RN setups (shows 4.3.16 is the published version in use). [3]
Would you like links to the exact GitHub issues or help troubleshooting a specific error?
Address known react-native-callkeep@4.3.16 compatibility issues with React Native 0.79.
This version has reported runtime and behavior problems on both Android and iOS with React Native 0.79, including Android behavior issues and iOS module problems with the New Architecture. Review the related GitHub issues in the react-native-callkeep repository and test thoroughly before merging, or consider using a newer compatible version if available.
🧰 Tools
🪛 GitHub Actions: Format Code with Prettier
[error] yarn install --frozen-lockfile failed. Package "" refers to a non-existing file '/Users/diegomello/Development/Work/Rocket.Chat/packages/media-signaling'.
🤖 Prompt for AI Agents
In `@package.json` at line 95, The package.json currently pins
react-native-callkeep to "react-native-callkeep": "^4.3.16", which is known to
have runtime compatibility issues with React Native 0.79; update the dependency
to a version proven compatible (or remove/replace it) and run full Android/iOS
smoke tests (including New Architecture on iOS) before merging — locate the
dependency entry "react-native-callkeep" in package.json, check upstream
issues/PRs for a recommended version or patch, change the version spec
accordingly (or replace with an alternate package), then run yarn/npm install
and platform-specific tests to verify fixes.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@app/sagas/login.js`:
- Around line 269-278: The startVoipFork saga currently calls simulateCall(),
which is test/debug-only and must not run in production; update startVoipFork so
it no longer invokes simulateCall unconditionally—either remove the
simulateCall() call entirely from startVoipFork or guard it behind a development
check (e.g., if (__DEV__) { simulateCall(); }) so production builds never
execute it; keep the rest of the flow (call initCallKeep, select user id,
mediaSessionInstance.init(userId)) unchanged.
- Around line 257-258: The saga is calling RNCallKeep.setup(options) and
RNCallKeep.canMakeMultipleCalls(false) without yielding their returned Promises;
change both calls to be yielded so the saga waits for completion (e.g., use
redux-saga yield call to invoke RNCallKeep.setup with options and yield call to
invoke RNCallKeep.canMakeMultipleCalls with false) so subsequent CallKeep
operations run only after these async operations finish.
♻️ Duplicate comments (2)
app/sagas/login.js (2)
260-263: Remove debugsetIntervalbefore merging.This interval logs every second indefinitely and will run in production, polluting logs and wasting resources. It appears to be leftover debug code.
Proposed fix: remove debug code
RNCallKeep.setup(options); RNCallKeep.canMakeMultipleCalls(false); - - const start = Date.now(); - setInterval(() => { - console.log('Timer fired after', Date.now() - start, 'ms'); - }, 1000); } catch (e) { log(e); }
274-274: Async functionsimulateCall()not properly yielded in saga.
simulateCall()is an async function but is called withoutyield call(). This means the saga won't wait for it to complete and any errors won't be caught by the try/catch block.Proposed fix: use call effect for async function
- simulateCall(); + yield call(simulateCall);
🧹 Nitpick comments (1)
app/sagas/login.js (1)
296-297: Arbitrary 1-second delay before VOIP initialization needs justification.The hardcoded
delay(1000)before starting VOIP lacks documentation explaining why it's necessary. If it's a workaround for a race condition, consider:
- Adding a comment explaining the purpose
- Using a more robust synchronization mechanism (e.g., waiting for a specific event or state)
- Ensuring the delay is sufficient across different device performance levels
Suggested improvement: add explanatory comment or remove if unnecessary
yield fork(fetchUsersRoles); - yield delay(1000); + // TODO: Document why this delay is needed or replace with proper synchronization + yield delay(1000); yield fork(startVoipFork);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
app/sagas/login.jsapp/views/CallView/components/CallActionButton.test.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- app/views/CallView/components/CallActionButton.test.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
app/sagas/login.js (2)
app/lib/services/voip/MediaSessionInstance.ts (1)
mediaSessionInstance(187-187)app/lib/services/voip/simulateCall.ts (1)
simulateCall(28-106)
🔇 Additional comments (2)
app/sagas/login.js (2)
5-6: LGTM!The imports for
RNCallKeepandPermissionsAndroidare appropriate for the VOIP integration.
44-45: LGTM!The VOIP-related imports align with the new initialization logic.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
Android Build Available Rocket.Chat Experimental 4.69.0.108136 Internal App Sharing: https://play.google.com/apps/test/RQVpXLytHNc/ahAO29uNSZyIJxuUHVw5xneNQoB90tJlcdPj3KC9EZAC0RFVhEwZ8o0Z9WbXA3ZPewi90iGdqlNLQy5AUAzrCdwkFU |
|
iOS Build Available Rocket.Chat Experimental 4.69.0.108137 |
- Implemented VoIP push notification handling in index.js, including storing call info for later processing. - Added CallKeep event handlers for answering and ending calls from a cold start. - Introduced a new CallIdUUID module to convert call IDs to deterministic UUIDs for compatibility with CallKit. - Created a pending call store to manage incoming calls when the app is not fully initialized. - Updated deep linking actions to include VoIP call handling. - Enhanced MediaSessionInstance to process pending calls and manage call states effectively.
diegolmello
force-pushed
the
feat.voip-lib-new
branch
from
bdb4b2a to
58e91f1
Compare
… + true-sheet + iOS 26) (#7114)
….ReactNative into feat.voip-lib-new
diegolmello
force-pushed
the
feat.voip-lib-new
branch
from
2143aba to
2a098f7
Compare
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