Conversation
📝 Walkthrough🚥 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)
645-668:⚠️ Potential issue | 🟠 MajorRoom instance leaks on connection failure due to missing cleanup in catch block.
The
roomobject created at line 451 with registered event listeners (lines 454-479) is only saved to state after a successfulconnect()call. If connection fails or any subsequent operation before state commit fails, the partially-initialized room is never cleaned up, leaking listeners and WebRTC resources.The proposed fix has a critical scoping bug:
roomis declared withconstinside the try block and is inaccessible in the catch block. The fix requires declaringroomoutside the try block:Corrected fix — declare room before try block and disconnect on failure
// Disconnect from current room if connected if (currentRoom) { currentRoom.disconnect(); } + let room: Room | null = null; + // Start the native audio session before connecting (required for production builds) // In dev builds, the audio session may persist across hot reloads, but in production // cold starts it must be explicitly started for WebRTC to function correctly if (Platform.OS !== 'web') { try { await AudioSession.startAudioSession(); logger.info({ message: 'Audio session started successfully', }); } catch (audioSessionError) { logger.warn({ message: 'Failed to start audio session - continuing with connection attempt', context: { error: audioSessionError }, }); } } // Create a new room - const room = new Room(); + room = new Room(); // Setup room event listeners room.on(RoomEvent.ParticipantConnected, (participant) => {And in the catch block:
} catch (error) { logger.error({ message: 'Failed to connect to room', context: { error, roomName: roomInfo?.Name }, }); + // Clean up the partially-created room to release event listeners and WebRTC resources + if (room) { + try { + await room.disconnect(); + } catch { + // room may not have connected — ignore + } + } + // Stop audio session on failure since we started it above🤖 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 645 - 668, The catch block leaks the LiveKit Room because the room variable is declared inside the try so it can't be cleaned up on error; move the room declaration (the variable used when calling connect() and registering listeners — the same identifier created around the connect flow) to outer scope before the try, and in the catch ensure you call the room.disconnect() (or Room.disconnect/close equivalent) and remove any registered event listeners for that room instance before exiting the error path; keep the existing AudioSession.stopAudioSession() and set({ isConnecting: false }) behavior but add the room cleanup and guard for room being non-null to avoid the scoping bug.
🧹 Nitpick comments (1)
src/stores/app/livekit-store.ts (1)
397-408: Hardcoded English strings in Alert calls — missing i18n.Lines 397, 407, 421-424, and 667 use hardcoded English strings in
Alert.alert(). As per coding guidelines, "Ensure all text is wrapped int()fromreact-i18nextfor translations with dictionary files stored insrc/translations."Also applies to: 421-425, 667-667
🤖 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 397 - 408, The Alert.alert calls in livekit-store.ts (the blocks checking for missing server address and missing token, and the other occurrences noted at 421-425 and 667) use hardcoded English strings; replace those plain strings with translation lookups using t(...) from react-i18next (or i18n.t(...) if you prefer non-hook usage in this store), e.g. t('voice.connection_error') and t('voice.missing_token') style keys, and add the corresponding keys/values to the JSON translation files under src/translations; ensure you import or access the translator (e.g., const { t } = useTranslation() or import i18n) at the top of the module and update each Alert.alert call to use the translation keys instead of literal English text.
🤖 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 428-431: The code calls currentRoom.disconnect() without awaiting
it, risking overlapping teardown with the subsequent connectToRoom logic; change
the disconnect call in the connectToRoom flow to await currentRoom.disconnect()
(i.e., await currentRoom.disconnect()) so the old Room's Promise completes
before proceeding—follow the same pattern used in disconnectFromRoom and other
callers to ensure transports/listeners are fully torn down prior to starting the
new Room connection.
---
Outside diff comments:
In `@src/stores/app/livekit-store.ts`:
- Around line 645-668: The catch block leaks the LiveKit Room because the room
variable is declared inside the try so it can't be cleaned up on error; move the
room declaration (the variable used when calling connect() and registering
listeners — the same identifier created around the connect flow) to outer scope
before the try, and in the catch ensure you call the room.disconnect() (or
Room.disconnect/close equivalent) and remove any registered event listeners for
that room instance before exiting the error path; keep the existing
AudioSession.stopAudioSession() and set({ isConnecting: false }) behavior but
add the room cleanup and guard for room being non-null to avoid the scoping bug.
---
Nitpick comments:
In `@src/stores/app/livekit-store.ts`:
- Around line 397-408: The Alert.alert calls in livekit-store.ts (the blocks
checking for missing server address and missing token, and the other occurrences
noted at 421-425 and 667) use hardcoded English strings; replace those plain
strings with translation lookups using t(...) from react-i18next (or i18n.t(...)
if you prefer non-hook usage in this store), e.g. t('voice.connection_error')
and t('voice.missing_token') style keys, and add the corresponding keys/values
to the JSON translation files under src/translations; ensure you import or
access the translator (e.g., const { t } = useTranslation() or import i18n) at
the top of the module and update each Alert.alert call to use the translation
keys instead of literal English text.
There was a problem hiding this comment.
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)
645-668:⚠️ Potential issue | 🟠 MajorCatch block may stop the audio session for an already-connected room.
After line 507, the room is committed to the store as connected (
isConnected: true). If any unguardedawaitbetween lines 507 and 644 throws (e.g.,playConnectToAudioRoomSoundat line 579), the catch block at line 652-661 stops the audio session — but the room remains connected in the store. This leaves WebRTC running without an active audio session.Consider either (a) wrapping post-commit calls like
playConnectToAudioRoomSoundin their own try-catch (consistent with other post-commit calls), or (b) skippingAudioSession.stopAudioSession()if the room was already committed.Proposed fix — wrap the sound call in its own try-catch
// Setup audio routing based on selected devices // This may change audio modes/focus, so it comes after media button init await setupAudioRouting(room); - await audioService.playConnectToAudioRoomSound(); + try { + await audioService.playConnectToAudioRoomSound(); + } catch (soundError) { + logger.warn({ + message: 'Failed to play connection sound', + context: { error: soundError }, + }); + }🤖 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 645 - 668, The catch currently stops the AudioSession unconditionally which can tear down audio for a room already committed as connected; to fix, wrap any post-commit awaits (e.g., playConnectToAudioRoomSound and similar calls between the commit/set that marks isConnected true and the try/catch boundary) in their own try/catch blocks so failures there don’t fall into the outer catch, and/or add a guard before calling AudioSession.stopAudioSession() that checks the store’s current connection state (e.g., read isConnected/room state or roomInfo) and skip stopping the audio session if the room was already marked connected.
🧹 Nitpick comments (1)
src/stores/app/livekit-store.ts (1)
397-397: User-facing strings are not wrapped int()for i18n.Multiple
Alert.alertcalls in the changed lines use hardcoded English strings (e.g.,'Voice Connection Error','Voice Connection Failed'). This is a pre-existing pattern in the file, but the changed lines introduce additional instances.As per coding guidelines, "Ensure all text is wrapped in
t()fromreact-i18nextfor translations with dictionary files stored insrc/translations".Also applies to: 407-407, 421-424, 667-667
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stores/app/livekit-store.ts` at line 397, The hardcoded user-facing strings passed to Alert.alert (e.g., 'Voice Connection Error', 'Voice server address is not available. Please try again later.', 'Voice Connection Failed', etc.) must be wrapped with the i18n translator t() from react-i18next; update each Alert.alert invocation in this file (search for Alert.alert usages around the instances noted) to call t('your.key') for the title and message (and add appropriate keys to the translations files in src/translations), import and use the t function where needed (or obtain it from the existing i18n context/hooks used elsewhere in this module) so all displayed text is localized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/stores/app/livekit-store.ts`:
- Around line 645-668: The catch currently stops the AudioSession
unconditionally which can tear down audio for a room already committed as
connected; to fix, wrap any post-commit awaits (e.g.,
playConnectToAudioRoomSound and similar calls between the commit/set that marks
isConnected true and the try/catch boundary) in their own try/catch blocks so
failures there don’t fall into the outer catch, and/or add a guard before
calling AudioSession.stopAudioSession() that checks the store’s current
connection state (e.g., read isConnected/room state or roomInfo) and skip
stopping the audio session if the room was already marked connected.
---
Duplicate comments:
In `@src/stores/app/livekit-store.ts`:
- Around line 428-431: The existing code did not await the
currentRoom.disconnect() call which could leave WebRTC transports and listeners
active during a new connection; update the disconnect path to await
currentRoom.disconnect() so the teardown completes before proceeding—make this
change where currentRoom is handled (referencing currentRoom.disconnect()) and
ensure it mirrors the awaited behavior used in disconnectFromRoom().
---
Nitpick comments:
In `@src/stores/app/livekit-store.ts`:
- Line 397: The hardcoded user-facing strings passed to Alert.alert (e.g.,
'Voice Connection Error', 'Voice server address is not available. Please try
again later.', 'Voice Connection Failed', etc.) must be wrapped with the i18n
translator t() from react-i18next; update each Alert.alert invocation in this
file (search for Alert.alert usages around the instances noted) to call
t('your.key') for the title and message (and add appropriate keys to the
translations files in src/translations), import and use the t function where
needed (or obtain it from the existing i18n context/hooks used elsewhere in this
module) so all displayed text is localized.
Member
Author
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Performance