Conversation
📝 WalkthroughWalkthroughAdds a Jest mock module for LiveKit React Native and enhances the LiveKit store connection flow with VoIP config checks, microphone permission prompts, native AudioSession lifecycle management (start/stop), foreground service handling, and user-facing error alerts. Changes
Sequence DiagramsequenceDiagram
actor User
participant Store as LiveKit Store
participant Permissions as Permission Handler
participant AudioSession as AudioSession
participant LiveKit as LiveKit Connection
participant FGService as Foreground Service
User->>Store: Request join voice channel
Store->>Permissions: Check microphone permission
Permissions-->>Store: Granted / Denied
alt Permission denied
Store->>User: Show permission alert (offer settings)
else Permission granted
Store->>Store: Validate VoIP server & token
alt Config invalid
Store->>User: Show config error alert
else Config valid
Store->>AudioSession: startAudioSession()
AudioSession-->>Store: started / error
alt AudioSession started / ok
Store->>FGService: start foreground service (Android)
FGService-->>Store: service started / error
Store->>LiveKit: connectToRoom()
LiveKit-->>Store: connected / error
alt Connected
Store->>Store: init CallKeep, set audio routing
else Connection failed
Store->>AudioSession: stopAudioSession()
Store->>User: Show connection error alert
end
else Start failed
Store->>User: Show audio session start error
end
end
end
alt On disconnect
Store->>AudioSession: stopAudioSession()
Store->>FGService: stop foreground service
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/stores/app/livekit-store.ts (1)
647-666:⚠️ Potential issue | 🟠 MajorFix type mismatch:
disconnectFromRoomisasyncbut typed as() => void, causing un-awaited cleanup with audio session lifecycle.The interface at line 185 declares
disconnectFromRoom: () => void, but the implementation isasync. Three call sites do not await: the CallKeep callback (line 523), LiveKitCallModal.handleLeaveRoom (line 56), and livekit-bottom-sheet.handleDisconnect (line 144). With the newAudioSession.stopAudioSession()andcallKeepService.endCall()cleanup, un-awaited disconnects create a race condition—if a user ends the call via system UI or modal and reconnects immediately, the old audio session may still be active.
- Update the interface:
disconnectFromRoom: () => Promise<void>;- Await the disconnect in LiveKitCallModal (line 56) and livekit-bottom-sheet (line 144)
- For the CallKeep callback (line 523), ensure the callback can be async or refactor the cleanup logic
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stores/app/livekit-store.ts` around lines 647 - 666, The disconnectFromRoom implementation is async but the declared type is () => void, causing callers (CallKeep callback, LiveKitCallModal.handleLeaveRoom, livekit-bottom-sheet.handleDisconnect) to not await cleanup and permit audio session races; change the store interface declaration for disconnectFromRoom to return Promise<void>, update all call sites to await get().disconnectFromRoom() in LiveKitCallModal.handleLeaveRoom and livekit-bottom-sheet.handleDisconnect, and modify the CallKeep callback (or refactor callKeepService.endCall flow) so it can await disconnectFromRoom (or invokes an async wrapper) ensuring AudioSession.stopAudioSession() and callKeepService.endCall() complete before proceeding.
🧹 Nitpick comments (1)
src/stores/app/livekit-store.ts (1)
385-388: Hardcoded user-facing strings should uset()for i18n.All
Alert.alertcalls added in this PR (lines 385, 400, 409, 639–643) use hardcoded English strings. The same pattern exists in the older alerts above (e.g., lines 339–348), so this is a pre-existing gap, but new code shouldn't widen it. As per coding guidelines: "Ensure all text is wrapped int()fromreact-i18nextfor translations."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stores/app/livekit-store.ts` around lines 385 - 388, The Alert.alert call is using hardcoded English strings; replace the title and message and button labels with i18n keys using t() from react-i18next (e.g., t('voiceConnectionError.title'), t('voiceConnectionError.message'), t('common.cancel'), t('common.openSettings')) and ensure t is available in this module (import/use the i18n hook or exported t) and pass the translated strings into Alert.alert and the onPress for Linking.openSettings remains unchanged; update all similar Alert.alert occurrences (including those at lines referenced) to use corresponding t() keys.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/stores/app/livekit-store.ts`:
- Around line 413-416: The code currently calls currentRoom.disconnect()
directly which bypasses the full cleanup flow; replace that direct call with the
store's full disconnect handler by calling get().disconnectFromRoom() (or
otherwise delegating to disconnectFromRoom) so AudioSession.stopAudioSession(),
CallKeep end, foreground service stop, callback teardown, and state reset run;
locate the currentRoom usage in this function and swap the direct disconnect to
invoke disconnectFromRoom() on the store to ensure complete cleanup.
---
Outside diff comments:
In `@src/stores/app/livekit-store.ts`:
- Around line 647-666: The disconnectFromRoom implementation is async but the
declared type is () => void, causing callers (CallKeep callback,
LiveKitCallModal.handleLeaveRoom, livekit-bottom-sheet.handleDisconnect) to not
await cleanup and permit audio session races; change the store interface
declaration for disconnectFromRoom to return Promise<void>, update all call
sites to await get().disconnectFromRoom() in LiveKitCallModal.handleLeaveRoom
and livekit-bottom-sheet.handleDisconnect, and modify the CallKeep callback (or
refactor callKeepService.endCall flow) so it can await disconnectFromRoom (or
invokes an async wrapper) ensuring AudioSession.stopAudioSession() and
callKeepService.endCall() complete before proceeding.
---
Nitpick comments:
In `@src/stores/app/livekit-store.ts`:
- Around line 385-388: The Alert.alert call is using hardcoded English strings;
replace the title and message and button labels with i18n keys using t() from
react-i18next (e.g., t('voiceConnectionError.title'),
t('voiceConnectionError.message'), t('common.cancel'), t('common.openSettings'))
and ensure t is available in this module (import/use the i18n hook or exported
t) and pass the translated strings into Alert.alert and the onPress for
Linking.openSettings remains unchanged; update all similar Alert.alert
occurrences (including those at lines referenced) to use corresponding t() keys.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/stores/app/livekit-store.ts (1)
647-666:⚠️ Potential issue | 🟠 MajorUpdate
disconnectFromRoomsignature from() => voidto() => Promise<void>and await all call sites.The implementation performs multiple awaited async operations (
currentRoom.disconnect(),AudioSession.stopAudioSession(),callKeepService.endCall(),notifee.stopForegroundService()), but the interface still declares() => void. This causes three call sites to skip awaiting, creating race conditions:
- Line 523 (CallKeep callback):
get().disconnectFromRoom()– cleanup may not complete before callback returns- LiveKitCallModal line 56 (
handleLeaveRoom):actions.disconnectFromRoom()followed byonClose()– modal closes before cleanup finishes- livekit-bottom-sheet line 144 (
handleDisconnect):disconnectFromRoom()followed bysetCurrentView()– state changes before cleanup is doneUpdate the interface to match the implementation and await these three call sites.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stores/app/livekit-store.ts` around lines 647 - 666, The disconnectFromRoom implementation is async but its type and callers still treat it as synchronous; change the disconnectFromRoom signature from () => void to () => Promise<void> (update the store/interface declaration where disconnectFromRoom is typed) and ensure all awaited async operations inside (currentRoom.disconnect(), AudioSession.stopAudioSession(), callKeepService.endCall(), notifee.stopForegroundService()) remain awaited. Then update all call sites to await it: the CallKeep callback that calls get().disconnectFromRoom(), the LiveKitCallModal handleLeaveRoom that calls actions.disconnectFromRoom() before onClose(), and the livekit-bottom-sheet handleDisconnect that calls disconnectFromRoom() before setCurrentView(); make these callers async and prepend await to the disconnectFromRoom calls so cleanup completes before continuing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/stores/app/livekit-store.ts`:
- Around line 385-411: Replace hardcoded Alert.alert messages with localized
strings by calling t() from react-i18next: wrap each user-facing title and body
string in t() using descriptive translation keys (e.g., 'voice.connectionError',
'voice.serverAddressUnavailable', 'voice.tokenMissing',
'voice.microphonePermissionRequired') and ensure the same keys are added to the
src/translations dictionary files; update the Alert.alert calls in
livekit-store.ts (the microphone permission block, the
voipServerWebsocketSslAddress check, and the token check) to use t('...') for
both title and message instead of raw English text.
- Around line 618-633: The connection error handler is too broad and treats
post-connect failures (e.g., audio routing, sound playback) as a room connection
failure; fix by distinguishing true connect failures from post-connect
side-effect errors: after the LiveKit connect step completes successfully set an
explicit flag (e.g., connectionConfirmed or set isConnected = true) before
running post-connect operations and move the logger.error/connection-failure
alert to only trigger when that flag is not set, and/or wrap each post-connect
call (playConnectToAudioRoomSound -> playSound, AudioSession.stopAudioSession
calls, setupAudioRouting, foreground service, CallKeep) in their own try/catch
so they log warnings without re-throwing or triggering the "Failed to connect to
room" path.
---
Outside diff comments:
In `@src/stores/app/livekit-store.ts`:
- Around line 647-666: The disconnectFromRoom implementation is async but its
type and callers still treat it as synchronous; change the disconnectFromRoom
signature from () => void to () => Promise<void> (update the store/interface
declaration where disconnectFromRoom is typed) and ensure all awaited async
operations inside (currentRoom.disconnect(), AudioSession.stopAudioSession(),
callKeepService.endCall(), notifee.stopForegroundService()) remain awaited. Then
update all call sites to await it: the CallKeep callback that calls
get().disconnectFromRoom(), the LiveKitCallModal handleLeaveRoom that calls
actions.disconnectFromRoom() before onClose(), and the livekit-bottom-sheet
handleDisconnect that calls disconnectFromRoom() before setCurrentView(); make
these callers async and prepend await to the disconnectFromRoom calls so cleanup
completes before continuing.
| Alert.alert('Voice Connection Error', 'Microphone permission is required to join a voice channel. Please grant the permission in your device settings.', [ | ||
| { text: 'Cancel', style: 'cancel' }, | ||
| { text: 'Open Settings', onPress: () => Linking.openSettings() }, | ||
| ]); | ||
| return; | ||
| } | ||
|
|
||
| const { currentRoom, voipServerWebsocketSslAddress } = get(); | ||
|
|
||
| // Disconnect from current room if connected | ||
| // Validate connection parameters before attempting to connect | ||
| if (!voipServerWebsocketSslAddress) { | ||
| logger.error({ | ||
| message: 'Cannot connect to room - no VoIP server address available', | ||
| context: { roomName: roomInfo.Name }, | ||
| }); | ||
| Alert.alert('Voice Connection Error', 'Voice server address is not available. Please try again later.'); | ||
| return; | ||
| } | ||
|
|
||
| if (!token) { | ||
| logger.error({ | ||
| message: 'Cannot connect to room - no token provided', | ||
| context: { roomName: roomInfo.Name }, | ||
| }); | ||
| Alert.alert('Voice Connection Error', 'Voice channel token is missing. Please try refreshing the voice channels.'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Localize new user-facing Alert strings.
These new alerts introduce hardcoded English strings; route them through t() with translation keys so they’re localized.
💡 Example adjustment
- Alert.alert('Voice Connection Error', 'Microphone permission is required to join a voice channel. Please grant the permission in your device settings.', [
+ Alert.alert(t('voice.connectionErrorTitle'), t('voice.micPermissionRequired'), [
{ text: t('common.cancel'), style: 'cancel' },
{ text: t('common.openSettings'), onPress: () => Linking.openSettings() },
]);As per coding guidelines, "Ensure all text is wrapped in t() from react-i18next for translations with dictionary files stored in src/translations".
Also applies to: 638-642
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/stores/app/livekit-store.ts` around lines 385 - 411, Replace hardcoded
Alert.alert messages with localized strings by calling t() from react-i18next:
wrap each user-facing title and body string in t() using descriptive translation
keys (e.g., 'voice.connectionError', 'voice.serverAddressUnavailable',
'voice.tokenMissing', 'voice.microphonePermissionRequired') and ensure the same
keys are added to the src/translations dictionary files; update the Alert.alert
calls in livekit-store.ts (the microphone permission block, the
voipServerWebsocketSslAddress check, and the token check) to use t('...') for
both title and message instead of raw English text.
| logger.error({ | ||
| message: 'Failed to connect to room', | ||
| context: { error }, | ||
| context: { error, roomName: roomInfo?.Name }, | ||
| }); | ||
|
|
||
| // Stop audio session on failure since we started it above | ||
| if (Platform.OS !== 'web') { | ||
| try { | ||
| await AudioSession.stopAudioSession(); | ||
| } catch (stopError) { | ||
| logger.warn({ | ||
| message: 'Failed to stop audio session after connection error', | ||
| context: { error: stopError }, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's see the structure of livekit-store.ts and find the connectToRoom method
fd -a "livekit-store.ts" src/Repository: Resgrid/Unit
Length of output: 106
🏁 Script executed:
# Get the full connectToRoom method to understand the complete flow
wc -l src/stores/app/livekit-store.tsRepository: Resgrid/Unit
Length of output: 91
🏁 Script executed:
# Search for the connectToRoom method and its context
rg -n "connectToRoom|async.*connectToRoom" src/stores/app/livekit-store.ts -A 150 | head -200Repository: Resgrid/Unit
Length of output: 8313
🏁 Script executed:
# Check for any connected flag or state tracking
rg -n "connected|isConnected|connectionStatus" src/stores/app/livekit-store.ts -B 2 -A 2Repository: Resgrid/Unit
Length of output: 3428
🏁 Script executed:
# Look at the audio service for error handling as indicated in the original request
fd -a "audio.service" src/Repository: Resgrid/Unit
Length of output: 380
🏁 Script executed:
# Get the complete connectToRoom method from line 372 onwards
sed -n '372,640p' src/stores/app/livekit-store.ts | cat -nRepository: Resgrid/Unit
Length of output: 12776
🏁 Script executed:
# Check the audio.service.ts for error handling
cat -n src/services/audio.service.ts | head -100Repository: Resgrid/Unit
Length of output: 4057
🏁 Script executed:
# Get the complete catch block to see all error handling
sed -n '617,680p' src/stores/app/livekit-store.ts | cat -nRepository: Resgrid/Unit
Length of output: 2477
🏁 Script executed:
# Find and inspect setupAudioRouting function
rg -n "setupAudioRouting" src/stores/app/livekit-store.ts -B 2 -A 20Repository: Resgrid/Unit
Length of output: 2265
🏁 Script executed:
# Check audio.service.ts for error handling in playConnectToAudioRoomSound
rg -n "playConnectToAudioRoomSound" src/services/audio.service.ts -B 2 -A 15Repository: Resgrid/Unit
Length of output: 774
🏁 Script executed:
# Find playSound implementation in audio.service.ts
rg -n "playSound" src/services/audio.service.ts -B 2 -A 20 | head -100Repository: Resgrid/Unit
Length of output: 2295
Consider guarding post-connect side effects from room connection status.
The outer try/catch will treat any later error (audio routing, sound playback) as a connection failure and show a misleading "Connection Failed" alert, even though the room is already connected and set to isConnected: true. While most operations are currently guarded (setupAudioRouting, foreground service, and CallKeep all have inner try/catch blocks), playConnectToAudioRoomSound() delegates to playSound() which silently logs failures without re-throwing. However, future post-connect operations without inner error handling would trigger the false-failure behavior. Consider either:
- Wrapping each post-connect operation individually to prevent false-failure alerts, or
- Using an explicit connection confirmation flag to distinguish actual connect failures from post-connect errors
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/stores/app/livekit-store.ts` around lines 618 - 633, The connection error
handler is too broad and treats post-connect failures (e.g., audio routing,
sound playback) as a room connection failure; fix by distinguishing true connect
failures from post-connect side-effect errors: after the LiveKit connect step
completes successfully set an explicit flag (e.g., connectionConfirmed or set
isConnected = true) before running post-connect operations and move the
logger.error/connection-failure alert to only trigger when that flag is not set,
and/or wrap each post-connect call (playConnectToAudioRoomSound -> playSound,
AudioSession.stopAudioSession calls, setupAudioRouting, foreground service,
CallKeep) in their own try/catch so they log warnings without re-throwing or
triggering the "Failed to connect to room" path.
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Tests