Develop #209
Develop #209
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| try { | ||
| // Generate a new UUID for this call | ||
| currentCallUUID = this.generateUUID(); |
Check failure
Code scanning / CodeQL
Insecure randomness High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 days ago
In general, the fix is to stop using Math.random() for generating the random bits of the UUID and instead use a cryptographically secure source of randomness. On React Native / JS environments, that means preferring global.crypto.getRandomValues (or crypto.randomUUID) and falling back to any available secure random byte function rather than Math.random().
The best targeted fix here is to keep the existing fast path using crypto.randomUUID() if present, but replace the Math.random-based fallback with a version that uses crypto.getRandomValues (or global.crypto.getRandomValues) to fill a byte array and then format those bytes into a UUID string. This preserves functionality (we still return a standards-like UUID string), but ensures that all randomness comes from a cryptographically secure generator whenever crypto is available. If crypto is not available at all, we can fall back to a deterministic but unique-enough identifier not based on Math.random (for example, timestamp plus an incrementing counter). That’s not cryptographically strong, but it removes the specific use of Math.random that CodeQL is detecting and is safer than pseudo-random values in a security context.
Concretely, in src/services/callkeep.service.android.ts:
- Update
generateUUIDto:- Try
global.crypto.randomUUID()(current behavior). - Else, if
global.crypto.getRandomValuesexists, generate a 16-byte buffer withgetRandomValuesand format it into a UUID of the formxxxxxxxx-xxxx-4xxx-yxxx-xxxxxxxxxxxxusing bit operations on those bytes only (no use ofMath.random()). - Else, fall back to a non-random but unique-ish identifier that does not use
Math.random, such as${Date.now()}-${++this.fallbackUUIDCounter}(introducing a private static or instance counter field). This keeps behavior reasonable even in very constrained environments while addressing the CodeQL finding.
- Try
No new external dependencies are required; we rely solely on the standard crypto object if present.
| @@ -278,15 +278,56 @@ | ||
| * Generate a UUID for CallKeep calls | ||
| */ | ||
| private generateUUID(): string { | ||
| // RN 0.76 typically provides global crypto.randomUUID via Hermes/JSI | ||
| // Prefer native/JSI crypto.randomUUID when available | ||
| const rndUUID = (global as any)?.crypto?.randomUUID?.(); | ||
| if (typeof rndUUID === 'string') return rndUUID; | ||
| // Fallback | ||
| return 'xxxxxxxx-xxxx-4xxx-yxxx-xxxxxxxxxxxx'.replace(/[xy]/g, (c) => { | ||
| const r = (Math.random() * 16) | 0; | ||
| const v = c === 'x' ? r : (r & 0x3) | 0x8; | ||
| return v.toString(16); | ||
| }); | ||
|
|
||
| const cryptoObj: any = (global as any)?.crypto; | ||
| if (cryptoObj && typeof cryptoObj.getRandomValues === 'function') { | ||
| // Secure fallback: use crypto.getRandomValues to generate RFC4122 v4-like UUID | ||
| const bytes = new Uint8Array(16); | ||
| cryptoObj.getRandomValues(bytes); | ||
|
|
||
| // Per RFC4122 section 4.4 | ||
| bytes[6] = (bytes[6] & 0x0f) | 0x40; // version 4 | ||
| bytes[8] = (bytes[8] & 0x3f) | 0x80; // variant 1 | ||
|
|
||
| const byteToHex: string[] = []; | ||
| for (let i = 0; i < 256; i++) { | ||
| byteToHex.push((i + 0x100).toString(16).substring(1)); | ||
| } | ||
|
|
||
| return ( | ||
| byteToHex[bytes[0]] + | ||
| byteToHex[bytes[1]] + | ||
| byteToHex[bytes[2]] + | ||
| byteToHex[bytes[3]] + | ||
| '-' + | ||
| byteToHex[bytes[4]] + | ||
| byteToHex[bytes[5]] + | ||
| '-' + | ||
| byteToHex[bytes[6]] + | ||
| byteToHex[bytes[7]] + | ||
| '-' + | ||
| byteToHex[bytes[8]] + | ||
| byteToHex[bytes[9]] + | ||
| '-' + | ||
| byteToHex[bytes[10]] + | ||
| byteToHex[bytes[11]] + | ||
| byteToHex[bytes[12]] + | ||
| byteToHex[bytes[13]] + | ||
| byteToHex[bytes[14]] + | ||
| byteToHex[bytes[15]] | ||
| ); | ||
| } | ||
|
|
||
| // Last-resort fallback: deterministic unique-ish ID without Math.random | ||
| // This should only be hit in very constrained environments without crypto. | ||
| const ts = Date.now().toString(16); | ||
| const pid = (typeof process !== 'undefined' && process.pid ? process.pid : 0).toString(16); | ||
| const counter = (this as any)._uuidCounter = (((this as any)._uuidCounter || 0) + 1); | ||
| const ctr = counter.toString(16); | ||
| return `${ts}-${pid}-${ctr}`; | ||
| } | ||
|
|
||
| /** |
📝 WalkthroughWalkthroughAdds Android CallKeep and an in-call audio native module, refactors audio playback/routing, tightens Bluetooth discovery/connection flows, updates LiveKit audio routing and permissions, UI/store adjustments for audio device selection and bottom-sheet navigation, dependency bumps, tests, and translation keys. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 🧪 Unit Test Generation v2 is now available!We have significantly improved our unit test generation capabilities. To enable: Add this to your reviews:
finishing_touches:
unit_tests:
enabled: trueTry it out by using the Have feedback? Share your thoughts on our Discord thread! 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: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/services/app-initialization.service.ts (1)
4-4: Use the platform-agnostic barrel export for CallKeep service.The platform check now supports both iOS and Android (line 92), but
callKeepServiceis imported directly fromcallkeep.service.ios. A platform-agnostic barrel export already exists atsrc/services/callkeep.service.tsthat correctly routes to the Android or iOS implementation based on the platform. Change the import on line 4 to:import { callKeepService } from './callkeep.service';This ensures Android devices use the Android-specific implementation instead of the iOS one. Other files in the codebase (e.g.,
bluetooth-audio.service.ts,livekit-store.ts) already follow this pattern.src/components/settings/audio-device-selection.tsx (1)
43-45: Missing translation for fallback device type label.The fallback at line 44 returns a dynamically constructed string that won't be translated. Per coding guidelines, all user-facing text should use
t().🌐 Proposed fix
default: - return deviceType.charAt(0).toUpperCase() + deviceType.slice(1) + ' Device'; + return t('settings.audio_device_selection.unknown_device_type', { type: deviceType });src/services/app-reset.service.ts (1)
223-241: Use replace flag to ensure complete state reset. ThesetStatecalls at lines 223, 235, 238, and 241 omit the replace flag, which causes Zustand to merge state instead of replace it. This leaves stale properties:
- useLiveKitStore:
isVoiceEnabled,voipServerWebsocketSslAddress,callerIdName,canConnectApiToken, andcanConnectToVoiceSessionare not inINITIAL_LIVEKIT_STATE, so they persist after reset.- useAudioStreamStore:
isLoadingStreamsandsoundObjectare not inINITIAL_AUDIO_STREAM_STATE, so they persist after reset.Change to
setState(INITIAL_*_STATE, true)to replace the entire state and ensure a clean reset after logout.
🤖 Fix all issues with AI agents
In `@app.config.ts`:
- Around line 83-85: Remove the unnecessary dangerous permission
'android.permission.READ_PHONE_NUMBERS' from the Android permissions array in
app.config.ts (where the manifest permissions list contains
'android.permission.READ_PHONE_STATE', 'android.permission.READ_PHONE_NUMBERS',
'android.permission.MANAGE_OWN_CALLS'); keep
'android.permission.READ_PHONE_STATE' and 'android.permission.MANAGE_OWN_CALLS'
for react-native-callkeep integration and only re-add READ_PHONE_NUMBERS if your
app explicitly uses TelephonyManager APIs to read the device phone number.
In `@expo-env.d.ts`:
- Line 3: The file expo-env.d.ts is missing a trailing newline which breaks
Prettier in CI; update expo-env.d.ts (the file containing the comment "// NOTE:
This file should not be edited and should be in your git ignore") by adding a
single newline character at the end of the file (ensure the file ends with a
blank line / newline) and commit the change so Prettier/CI accepts the file.
In `@package.json`:
- Line 171: The PR added the native dependency "react-native-sound" which
duplicates audio functionality already provided by "expo-audio"; remove
"react-native-sound" from package.json and refactor your audio service to use
the existing expo-audio APIs instead of native react-native-sound calls. Locate
imports and usage in files referencing "react-native-sound" (e.g., AudioService,
any play/pause/stop helper functions) and replace them with expo-audio
equivalents (update imports from "expo-audio" and adapt to its
loadAsync/playAsync/unloadAsync patterns), remove any native-only setup/config,
and update README/dependency notes accordingly so the project uses only
expo-audio. Ensure package.json and Metro/bundler config no longer reference
react-native-sound and run a build to verify no native modules remain.
In `@src/components/livekit/livekit-bottom-sheet.tsx`:
- Around line 150-176: Replace console.log and console.error in the
updateAudioRouting useEffect with the shared logger from "@/lib/logging" (use
logger.info or logger.debug for the 'Updating audio routing for:' message and
logger.error for failures) and keep references to selectedAudioDevices.speaker
and applyAudioRouting; then evaluate/remove this entire useEffect if it
duplicates setupAudioRouting from livekit-store (called during connectToRoom) to
avoid two competing routing flows—if you remove it, ensure connectToRoom →
setupAudioRouting covers all speaker type branches formerly handled by
updateAudioRouting.
In `@src/components/settings/bluetooth-device-selection-bottom-sheet.tsx`:
- Around line 1-5: The imports at the top of the Bluetooth device selection
component are out of order and failing simple-import-sort; run eslint --fix (or
manually reorder) to group and sort imports: external React and react-native
imports first (e.g., import React, { useCallback, useEffect, useState } from
'react'; import { Alert, useWindowDimensions } from 'react-native';), then
third-party hooks and libraries (e.g., import { useTranslation } from
'react-i18next'; import { showMessage } from 'react-native-flash-message';),
then icon imports (e.g., import { BluetoothIcon, RefreshCwIcon, WifiIcon } from
'lucide-react-native';), ensuring alphabetical order within each group so the
file-level imports in bluetooth-device-selection-bottom-sheet.tsx satisfy
simple-import-sort/imports.
- Around line 110-125: Remove the stray blank line after the onClose() call and
eliminate the redundant parentheses around the ternary fallback expression
passed to showMessage so the block reads compactly; specifically inside the
connect/handler function in bluetooth-device-selection-bottom-sheet.tsx (where
onClose(), logger.warn({...}), errorMessage is derived, and showMessage({...})
is called), collapse the extra newline after onClose() and change the
description value from (... ? ... : (errorMessage ||
t('bluetooth.connection_error_message') || 'Could not connect to device')) to
... ? ... : errorMessage || t('bluetooth.connection_error_message') || 'Could
not connect to device' so linting errors are resolved.
In `@src/services/__tests__/bluetooth-audio-b01inrico.test.ts`:
- Around line 191-203: The test block for the case "should ignore long press on
PTT stop (0x80)" has an extra closing brace causing a syntax error; open the
test containing the call to service.parseB01InricoButtonData(...) and remove the
duplicate closing brace so the single it(...) block is properly closed (ensure
only one "});" ends that test).
In `@src/services/audio.service.ts`:
- Around line 99-152: The Sound constructor loads asynchronously but you're not
awaiting it: wrap each new Sound(...) creation in a Promise that resolves when
the constructor callback reports success and rejects/logs on error (e.g., create
a helper like loadSound(uri): Promise<Sound>), replace the direct assignments to
this.startTransmittingSound, this.stopTransmittingSound,
this.connectedDeviceSound, this.connectToAudioRoomSound,
this.disconnectedFromAudioRoomSound with awaited calls to that helper (or use
Promise.all to load concurrently), and only set isInitialized = true after all
sound Promises have resolved so initialization truly waits for sounds to be
ready.
In `@src/services/bluetooth-audio.service.ts`:
- Around line 1794-1796: Remove the duplicated assignment to this.isInitialized
so it's only set once (leave the single initialization for this.isInitialized
and keep the existing this.hasAttemptedPreferredDeviceConnection
initialization); locate the consecutive lines where this.isInitialized = false
appears and delete the redundant one (e.g., in the initialization
block/constructor of the BluetoothAudioService where this.isInitialized and
this.hasAttemptedPreferredDeviceConnection are set).
- Around line 1259-1260: Replace the direct console.log call with the module's
logger instance: locate the console.log(`[PTT_DEBUG] Raw Buffer: ${rawHex} |
Bytes: ${allBytes}`) in bluetooth-audio.service.ts and change it to use
logger.debug (or the appropriate logger level) passing the same message and
variables (rawHex and allBytes) so logging follows the app's logger utility and
formatting conventions.
In `@src/stores/app/bluetooth-audio-store.ts`:
- Around line 148-155: The audio-device type strings changed from 'default' to
role-based values which breaks recovery logic; update the recovery filter in
bluetooth-audio.service (the code currently using d.type === 'default') to look
up devices by role (use d.type === 'microphone' when recovering microphone and
d.type === 'speaker' when recovering speaker), remove the unused 'default'
member from the type union in bluetooth-audio-store (the type for
availableAudioDevices/selectedAudioDevices), and update the test fixtures in
livekit-bottom-sheet.test.tsx to use 'microphone' and 'speaker' instead of
'default-mic'/'default-speaker' types so tests match the new role-based types.
In `@src/stores/app/livekit-store.ts`:
- Line 4: Remove the unused import RTCAudioSession from the top of the file;
locate the import statement that reads "import { RTCAudioSession } from
'@livekit/react-native-webrtc'" and delete RTCAudioSession (or remove the entire
import if nothing else is imported from that module) since the only usage is
commented out in the block around the commented lines referencing
RTCAudioSession (around the code that sets audio session behavior).
🧹 Nitpick comments (14)
src/services/audio.service.ts (2)
17-20: Misleading comment: all sounds now use react-native-sound.The comment says "Keep others as expo-av for now" but all five sound fields are typed as
Sound | nullfrom react-native-sound. Update or remove the comment to avoid confusion.💡 Suggested fix
- // Keep others as expo-av for now - private connectedDeviceSound: Sound | null = null; + private connectedDeviceSound: Sound | null = null;
169-187: Async methods don't await anything.The
playStartTransmittingSoundmethod is markedasyncbut doesn'tawaitanything. TheSound.play()API is callback-based, not promise-based. Consider either:
- Removing
asyncsince it's misleading, or- Wrapping the play call in a Promise if you need to wait for completion
This applies to all play methods in this file.
src/services/callkeep.service.android.ts (5)
6-7: Module-level state alongside instance state creates potential inconsistency.
currentCallUUIDis declared at module level but the service also tracksisCallActiveas an instance property. If somehow multiple instances existed (despite singleton pattern), or if the singleton were reset, these could become out of sync. Consider movingcurrentCallUUIDto be an instance property for consistency.♻️ Proposed refactor to use instance property
-// UUID for the CallKeep call - should be unique per session -let currentCallUUID: string | null = null; - export interface CallKeepConfig { appName: string; ... } export class CallKeepService { private static instance: CallKeepService | null = null; private isSetup = false; private isCallActive = false; private muteStateCallback: ((muted: boolean) => void) | null = null; + private currentCallUUID: string | null = null;Then update all references from
currentCallUUIDtothis.currentCallUUID.
54-66: iOS audio session options in Android-specific file appear intentional but could benefit from a comment.The
iosconfiguration block is included in the Android service file. This is likely required by theRNCallKeep.setup()API which expects both platform configurations. A brief comment clarifying this would improve maintainability.📝 Suggested clarification comment
try { + // RNCallKeep.setup() requires both ios and android config objects + // even when running on a single platform const options = { ios: {
212-214: Unused parameter inignoreMuteEvents.The
durationMsparameter is unused. While documented as a no-op for Android parity with iOS, consider prefixing with underscore to signal intent and satisfy linters.♻️ Proposed fix
- ignoreMuteEvents(durationMs: number): void { + ignoreMuteEvents(_durationMs: number): void { // No-op on Android }
305-308: Event listeners removed without verifying they were registered.If
cleanup()is called beforesetup()completes (or if setup failed), removing non-existent event listeners could potentially throw. Consider guarding withthis.isSetupcheck before removing listeners.🛡️ Proposed defensive check
// Remove event listeners + if (this.isSetup) { RNCallKeep.removeEventListener('endCall'); RNCallKeep.removeEventListener('answerCall'); RNCallKeep.removeEventListener('didPerformSetMutedCallAction'); + } this.isSetup = false;
280-290: Consider usingexpo-cryptofor more robust UUID generation instead ofMath.random()fallback.The
Math.random()fallback for CallKeep session identifiers can be improved by usingCrypto.randomUUID()fromexpo-crypto, which provides a cryptographically stronger implementation. While CallKeep UUIDs don't handle sensitive data, this approach is more robust and eliminates the use of weak randomness for ID generation.♻️ Proposed fix using expo-crypto
+import * as Crypto from 'expo-crypto'; + /** * Generate a UUID for CallKeep calls */ private generateUUID(): string { // RN 0.76 typically provides global crypto.randomUUID via Hermes/JSI const rndUUID = (global as any)?.crypto?.randomUUID?.(); if (typeof rndUUID === 'string') return rndUUID; - // Fallback - return 'xxxxxxxx-xxxx-4xxx-yxxx-xxxxxxxxxxxx'.replace(/[xy]/g, (c) => { - const r = (Math.random() * 16) | 0; - const v = c === 'x' ? r : (r & 0x3) | 0x8; - return v.toString(16); - }); + // Fallback using expo-crypto + return Crypto.randomUUID(); }Note: Requires installing
expo-cryptoif not already present:npx expo install expo-cryptosrc/components/settings/__tests__/audio-device-selection.test.tsx (1)
226-239: Test correctly updated to reflect stricter device filtering.The test now properly expects that unavailable wired devices are filtered out, matching the updated filtering logic that requires
isAvailablefor all device types. The comment at line 230 ("Should still show even if unavailable") appears to be stale since the assertion now expects it to be null.📝 Update stale comment
const availableBluetooth = createMockDevice('bt-available', 'Available BT', 'bluetooth', true); const unavailableBluetooth = createMockDevice('bt-unavailable', 'Unavailable BT', 'bluetooth', false); - const wiredDevice = createMockDevice('wired-1', 'Wired Device', 'wired', false); // Should still show even if unavailable + const wiredDevice = createMockDevice('wired-1', 'Wired Device', 'wired', false); // Should be filtered out when unavailable mockStore.availableAudioDevices = [availableBluetooth, unavailableBluetooth, wiredDevice];src/components/settings/audio-device-selection.tsx (1)
73-75: LGTM on the filtering logic enhancement.The updated filters now explicitly require
isAvailableand restrict to known device types (default,microphone/speaker,bluetooth,wired). This is a good improvement for clarity and correctness.Optional: Consider extracting the valid types into constants for maintainability:
♻️ Optional refactor for readability
+const MICROPHONE_TYPES = ['default', 'microphone', 'bluetooth', 'wired'] as const; +const SPEAKER_TYPES = ['default', 'speaker', 'bluetooth', 'wired'] as const; + -const availableMicrophones = availableAudioDevices.filter((device) => device.isAvailable && (device.type === 'default' || device.type === 'microphone' || device.type === 'bluetooth' || device.type === 'wired')); +const availableMicrophones = availableAudioDevices.filter((device) => device.isAvailable && MICROPHONE_TYPES.includes(device.type)); -const availableSpeakers = availableAudioDevices.filter((device) => device.isAvailable && (device.type === 'default' || device.type === 'speaker' || device.type === 'bluetooth' || device.type === 'wired')); +const availableSpeakers = availableAudioDevices.filter((device) => device.isAvailable && SPEAKER_TYPES.includes(device.type));,
src/stores/auth/store.tsx (1)
260-301: Appropriate deferral pattern for store initialization.Wrapping the rehydration logic in
setTimeout(..., 0)correctly ensures the store is fully initialized before scheduling token refresh. This addresses potential race conditions during app startup.Consider extracting duplicate timeout management logic:
The pattern of clearing existing timeout and scheduling a new one is repeated across login, refreshAccessToken, and both rehydration branches.
♻️ Optional refactor to reduce duplication
// Helper function to schedule token refresh with timeout management const scheduleTokenRefresh = (delayMs: number) => { const existingTimeoutId = useAuthStore.getState().refreshTimeoutId; if (existingTimeoutId !== null) { clearTimeout(existingTimeoutId); } const timeoutId = setTimeout(() => { useAuthStore.getState().refreshAccessToken(); }, delayMs); useAuthStore.setState({ refreshTimeoutId: timeoutId }); };,
src/components/settings/bluetooth-device-selection-bottom-sheet.tsx (2)
63-83: Remove commented-out code and inline TODO comments.Lines 63-83 contain development artifacts (commented-out disconnect logic and inline design notes). Clean these up before merging:
🧹 Proposed cleanup
async (device: BluetoothAudioDevice) => { try { - // Disconnect from any currently connected device first? - // The service connectToDevice usually handles this or we do it here. - // Previous code did disconnect manually. - /* - if (connectedDevice) { - ... disconnect ... - } - */ - // User wants "When attempting to connect... display loading". - // User wants "If error... don't save device". - - // 1. Set connecting state - // We can resolve this by using local state for the specific device being connected to, - // or rely on store's global isConnecting. - // Let's use the store's setIsConnecting to be consistent if the UI uses it. + // Set connecting state useBluetoothAudioStore.getState().setIsConnecting(true); setConnectingDeviceId(device.id); - - // 2. Clear existing preferred temporarily? Or just wait? - // User said "don't save the save device, only do that when connection is successful". - // So we shouldn't touch preferredDevice yet.
251-261: Good use ofextraDatafor connecting state; considergetItemLayoutfor better performance.The
extraData={connectingDeviceId}correctly ensures re-renders when the connecting state changes.For further optimization, consider adding
getItemLayoutsince device items have consistent height (~60px based onestimatedItemSize). Based on learnings: "Use getItemLayout for FlatList when items have consistent size."♻️ Optional performance improvement
+const ITEM_HEIGHT = 60; +const getItemLayout = (_: unknown, index: number) => ({ + length: ITEM_HEIGHT, + offset: ITEM_HEIGHT * index, + index, +}); + <FlatList data={availableDevices} renderItem={renderDeviceItem} keyExtractor={(item) => item.id} ListEmptyComponent={renderEmptyState} showsVerticalScrollIndicator={false} - estimatedItemSize={60} + getItemLayout={getItemLayout} extraData={connectingDeviceId} />,
src/stores/app/livekit-store.ts (1)
38-52: Remove or convert commented-out code to actionable TODO.Lines 38-49 contain commented-out
RTCAudioSessionlogic with explanatory comments. Either:
- Remove the dead code entirely, or
- Convert to a clear TODO comment if this is intended future work
🧹 Suggested cleanup
await Audio.setAudioModeAsync({ allowsRecordingIOS: true, staysActiveInBackground: true, playsInSilentModeIOS: true, shouldDuckAndroid: true, - // For speaker, we want false (speaker). For others, simple routing. playThroughEarpieceAndroid: deviceType !== 'speaker', interruptionModeIOS: InterruptionModeIOS.DoNotMix, }); - // Use RTCAudioSession to force route selection for WebRTC (Not available on Android in this package) - // We rely on Audio.setAudioModeAsync and system behavior. - /* - const RTCAudioSessionAny = RTCAudioSession as any; - if (RTCAudioSessionAny.getAudioDevices && RTCAudioSessionAny.selectAudioDevice) { - // ... (Logic removed as it's iOS only) - } else { - logger.info({ - message: 'RTCAudioSession Android methods not available (Relying on system routing)', - }); - } - */ + // Note: RTCAudioSession device selection APIs are iOS-only. + // Android relies on Audio.setAudioModeAsync and system behavior for routing. logger.info({ message: 'Android audio routing applied via Audio.setAudioModeAsync', });src/services/bluetooth-audio.service.ts (1)
2-2: Consider addressing the underlying type resolution issue.The
@ts-ignorecomment suggests a type resolution issue with the callkeep service import. This may indicate that the barrel file (@/services/callkeep.service) isn't properly exporting types or that the platform-specific implementations need unified type declarations.#!/bin/bash # Check how callkeep.service is exported and if there's a barrel file fd -t f "callkeep.service" --exec cat {} \; 2>/dev/null | head -50 # Check for an index file in the services directory cat src/services/index.ts 2>/dev/null || echo "No barrel file found"
| /// <reference types="expo/types" /> | ||
|
|
||
| // NOTE: This file should not be edited and should be in your git ignore | ||
| // NOTE: This file should not be edited and should be in your git ignore No newline at end of file |
There was a problem hiding this comment.
Add missing newline at end of file.
The CI pipeline is failing because Prettier requires a trailing newline. Add a newline after line 3.
🧰 Tools
🪛 GitHub Actions: React Native CI/CD
[warning] 3-3: prettier/prettier: Insert ⏎
🪛 GitHub Check: test
[warning] 3-3:
Insert ⏎
🤖 Prompt for AI Agents
In `@expo-env.d.ts` at line 3, The file expo-env.d.ts is missing a trailing
newline which breaks Prettier in CI; update expo-env.d.ts (the file containing
the comment "// NOTE: This file should not be edited and should be in your git
ignore") by adding a single newline character at the end of the file (ensure the
file ends with a blank line / newline) and commit the change so Prettier/CI
accepts the file.
package.json
Outdated
| "react-native-restart": "0.0.27", | ||
| "react-native-safe-area-context": "5.4.0", | ||
| "react-native-screens": "~4.11.1", | ||
| "react-native-sound": "^0.13.0", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all references to react-native-sound
rg "react-native-sound" -g '!node_modules'Repository: Resgrid/Unit
Length of output: 1148
🏁 Script executed:
#!/bin/bash
# Check for expo config files and plugins
find . -maxdepth 3 -type f \( -name "app.json" -o -name "eas.json" -o -name "app.config.*" \) | head -20Repository: Resgrid/Unit
Length of output: 82
🏁 Script executed:
#!/bin/bash
# Check if expo-av is already in package.json
grep -A 2 -B 2 '"expo-av"' package.jsonRepository: Resgrid/Unit
Length of output: 211
🏁 Script executed:
#!/bin/bash
# Search for audio-related code
rg "audio\." -g '!node_modules' --type ts --type js | head -30Repository: Resgrid/Unit
Length of output: 3404
🌐 Web query:
react-native-sound expo config plugin compatibility 2025
💡 Result:
react-native-soundis a native module, so it won’t work in Expo Go. To use it in an Expo app you need a custom development build / Dev Client and to generate native projects via CNG (expo prebuild) / EAS Build. (docs.expo.dev)- Config plugin status (2025): there doesn’t appear to be an official or commonly used community Expo config plugin specifically for
react-native-sound(it’s not among the community-maintained plugin repo’s catalog). In many cases, a library needs no config plugin at all—you just install it and rebuild—unless it requires extra native edits (Info.plist/AndroidManifest/AppDelegate, etc.). (github.com) - Practical recommendation for Expo apps: if you primarily need audio playback/recording and want “Expo-native” support (including a built-in config plugin), prefer
expo-audio(or other Expo audio libraries) instead ofreact-native-sound. (docs.expo.dev)
Citations:
- 1: https://docs.expo.dev/versions/latest/sdk/dev-client?utm_source=openai
- 2: https://github.com/expo/config-plugins?utm_source=openai
- 3: https://docs.expo.dev/versions/latest/sdk/audio/?utm_source=openai
Consider consolidating to expo-audio instead of adding react-native-sound.
react-native-sound requires native code and has no official Expo config plugin. While your project's Expo prebuild setup can handle it, you already have expo-audio (v0.4.9) as a dependency—which is purpose-built for Expo's managed workflow and fully supported by Expo SDK.
Evaluate whether expo-audio can cover your audio playback needs for the audio service instead of adding another native audio library. This will reduce native dependencies and align better with Expo's managed workflow approach.
🤖 Prompt for AI Agents
In `@package.json` at line 171, The PR added the native dependency
"react-native-sound" which duplicates audio functionality already provided by
"expo-audio"; remove "react-native-sound" from package.json and refactor your
audio service to use the existing expo-audio APIs instead of native
react-native-sound calls. Locate imports and usage in files referencing
"react-native-sound" (e.g., AudioService, any play/pause/stop helper functions)
and replace them with expo-audio equivalents (update imports from "expo-audio"
and adapt to its loadAsync/playAsync/unloadAsync patterns), remove any
native-only setup/config, and update README/dependency notes accordingly so the
project uses only expo-audio. Ensure package.json and Metro/bundler config no
longer reference react-native-sound and run a build to verify no native modules
remain.
| // Audio Routing Logic | ||
| useEffect(() => { | ||
| const updateAudioRouting = async () => { | ||
| if (!selectedAudioDevices.speaker) return; | ||
|
|
||
| try { | ||
| const speaker = selectedAudioDevices.speaker; | ||
| console.log('Updating audio routing for:', speaker.type); | ||
|
|
||
| let targetType: 'bluetooth' | 'speaker' | 'earpiece' | 'default' = 'default'; | ||
|
|
||
| if (speaker.type === 'speaker') { | ||
| targetType = 'speaker'; | ||
| } else if (speaker.type === 'bluetooth') { | ||
| targetType = 'bluetooth'; | ||
| } else { | ||
| targetType = 'earpiece'; | ||
| } | ||
|
|
||
| await applyAudioRouting(targetType); | ||
| } catch (error) { | ||
| console.error('Failed to update audio routing:', error); | ||
| } | ||
| }; | ||
|
|
||
| updateAudioRouting(); | ||
| }, [selectedAudioDevices.speaker]); |
There was a problem hiding this comment.
Replace console.log/error with logger for consistency.
The codebase uses logger from @/lib/logging throughout. Lines 157 and 171 use console.log/console.error instead.
Additionally, this audio routing logic appears duplicated with setupAudioRouting in src/stores/app/livekit-store.ts. Consider whether this effect is necessary given that setupAudioRouting is called during connectToRoom.
🔧 Proposed fix for logging
- console.log('Updating audio routing for:', speaker.type);
+ logger.debug({
+ message: 'Updating audio routing for speaker',
+ context: { speakerType: speaker.type },
+ });
...
- console.error('Failed to update audio routing:', error);
+ logger.error({
+ message: 'Failed to update audio routing',
+ context: { error },
+ });,
📝 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.
| // Audio Routing Logic | |
| useEffect(() => { | |
| const updateAudioRouting = async () => { | |
| if (!selectedAudioDevices.speaker) return; | |
| try { | |
| const speaker = selectedAudioDevices.speaker; | |
| console.log('Updating audio routing for:', speaker.type); | |
| let targetType: 'bluetooth' | 'speaker' | 'earpiece' | 'default' = 'default'; | |
| if (speaker.type === 'speaker') { | |
| targetType = 'speaker'; | |
| } else if (speaker.type === 'bluetooth') { | |
| targetType = 'bluetooth'; | |
| } else { | |
| targetType = 'earpiece'; | |
| } | |
| await applyAudioRouting(targetType); | |
| } catch (error) { | |
| console.error('Failed to update audio routing:', error); | |
| } | |
| }; | |
| updateAudioRouting(); | |
| }, [selectedAudioDevices.speaker]); | |
| // Audio Routing Logic | |
| useEffect(() => { | |
| const updateAudioRouting = async () => { | |
| if (!selectedAudioDevices.speaker) return; | |
| try { | |
| const speaker = selectedAudioDevices.speaker; | |
| logger.debug({ | |
| message: 'Updating audio routing for speaker', | |
| context: { speakerType: speaker.type }, | |
| }); | |
| let targetType: 'bluetooth' | 'speaker' | 'earpiece' | 'default' = 'default'; | |
| if (speaker.type === 'speaker') { | |
| targetType = 'speaker'; | |
| } else if (speaker.type === 'bluetooth') { | |
| targetType = 'bluetooth'; | |
| } else { | |
| targetType = 'earpiece'; | |
| } | |
| await applyAudioRouting(targetType); | |
| } catch (error) { | |
| logger.error({ | |
| message: 'Failed to update audio routing', | |
| context: { error }, | |
| }); | |
| } | |
| }; | |
| updateAudioRouting(); | |
| }, [selectedAudioDevices.speaker]); |
🤖 Prompt for AI Agents
In `@src/components/livekit/livekit-bottom-sheet.tsx` around lines 150 - 176,
Replace console.log and console.error in the updateAudioRouting useEffect with
the shared logger from "@/lib/logging" (use logger.info or logger.debug for the
'Updating audio routing for:' message and logger.error for failures) and keep
references to selectedAudioDevices.speaker and applyAudioRouting; then
evaluate/remove this entire useEffect if it duplicates setupAudioRouting from
livekit-store (called during connectToRoom) to avoid two competing routing
flows—if you remove it, ensure connectToRoom → setupAudioRouting covers all
speaker type branches formerly handled by updateAudioRouting.
| // FORCE LOG COMPATIBILITY | ||
| console.log(`[PTT_DEBUG] Raw Buffer: ${rawHex} | Bytes: ${allBytes}`); |
There was a problem hiding this comment.
Replace console.log with the logger utility.
Direct console.log usage violates the coding guidelines. Use the existing logger instance for consistent logging behavior across the application.
Proposed fix
- // FORCE LOG COMPATIBILITY
- console.log(`[PTT_DEBUG] Raw Buffer: ${rawHex} | Bytes: ${allBytes}`);
+ logger.debug({
+ message: '[PTT_DEBUG] Raw buffer data',
+ context: { rawHex, allBytes },
+ });📝 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.
| // FORCE LOG COMPATIBILITY | |
| console.log(`[PTT_DEBUG] Raw Buffer: ${rawHex} | Bytes: ${allBytes}`); | |
| logger.debug({ | |
| message: '[PTT_DEBUG] Raw buffer data', | |
| context: { rawHex, allBytes }, | |
| }); |
🤖 Prompt for AI Agents
In `@src/services/bluetooth-audio.service.ts` around lines 1259 - 1260, Replace
the direct console.log call with the module's logger instance: locate the
console.log(`[PTT_DEBUG] Raw Buffer: ${rawHex} | Bytes: ${allBytes}`) in
bluetooth-audio.service.ts and change it to use logger.debug (or the appropriate
logger level) passing the same message and variables (rawHex and allBytes) so
logging follows the app's logger utility and formatting conventions.
| this.isInitialized = false; | ||
| this.isInitialized = false; | ||
| this.hasAttemptedPreferredDeviceConnection = false; |
There was a problem hiding this comment.
Remove duplicate assignment.
this.isInitialized = false is assigned twice consecutively, which appears to be a copy-paste error.
Proposed fix
// Reset initialization flags
this.isInitialized = false;
- this.isInitialized = false;
this.hasAttemptedPreferredDeviceConnection = false;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.isInitialized = false; | |
| this.isInitialized = false; | |
| this.hasAttemptedPreferredDeviceConnection = false; | |
| this.isInitialized = false; | |
| this.hasAttemptedPreferredDeviceConnection = false; |
🤖 Prompt for AI Agents
In `@src/services/bluetooth-audio.service.ts` around lines 1794 - 1796, Remove the
duplicated assignment to this.isInitialized so it's only set once (leave the
single initialization for this.isInitialized and keep the existing
this.hasAttemptedPreferredDeviceConnection initialization); locate the
consecutive lines where this.isInitialized = false appears and delete the
redundant one (e.g., in the initialization block/constructor of the
BluetoothAudioService where this.isInitialized and
this.hasAttemptedPreferredDeviceConnection are set).
| availableAudioDevices: [ | ||
| { id: 'default-mic', name: 'Default Microphone', type: 'default', isAvailable: true }, | ||
| { id: 'default-mic', name: 'Default Microphone', type: 'microphone', isAvailable: true }, | ||
| { id: 'default-speaker', name: 'Default Speaker', type: 'speaker', isAvailable: true }, | ||
| ], | ||
| selectedAudioDevices: { | ||
| microphone: { id: 'default-mic', name: 'Default Microphone', type: 'default', isAvailable: true }, | ||
| microphone: { id: 'default-mic', name: 'Default Microphone', type: 'microphone', isAvailable: true }, | ||
| speaker: { id: 'default-speaker', name: 'Default Speaker', type: 'speaker', isAvailable: true }, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for code that filters audio devices by type 'default' which may need updating
rg -n "type.*['\"]default['\"]" --type ts -C2Repository: Resgrid/Unit
Length of output: 3409
🏁 Script executed:
# Search for microphone-related type filtering and device type usage
rg -n "type.*microphone|\.type\s*===?\s*['\"]" --type ts -C2Repository: Resgrid/Unit
Length of output: 9375
🏁 Script executed:
# Read the bluetooth-audio-store.ts file to understand the context
wc -l src/stores/app/bluetooth-audio-store.tsRepository: Resgrid/Unit
Length of output: 99
🏁 Script executed:
# Search for any references to availableAudioDevices or selectedAudioDevices to see how they're used
rg -n "availableAudioDevices|selectedAudioDevices" --type ts -C2Repository: Resgrid/Unit
Length of output: 24705
Update device type handling when changing from 'default' to role-based types.
The type change from 'default' to 'microphone' and 'speaker' breaks the audio device recovery logic in src/services/bluetooth-audio.service.ts (lines 1686-1687), which explicitly filters for type === 'default' when reverting to non-Bluetooth devices. Update the service to filter by role instead:
- Search for microphone:
d.type === 'microphone' - Search for speaker:
d.type === 'speaker'
Also update the type union in src/stores/app/bluetooth-audio-store.ts (line 46) to remove the unused 'default' value, and update test data in src/components/livekit/__tests__/livekit-bottom-sheet.test.tsx (lines 133-134) to use the new types.
🤖 Prompt for AI Agents
In `@src/stores/app/bluetooth-audio-store.ts` around lines 148 - 155, The
audio-device type strings changed from 'default' to role-based values which
breaks recovery logic; update the recovery filter in bluetooth-audio.service
(the code currently using d.type === 'default') to look up devices by role (use
d.type === 'microphone' when recovering microphone and d.type === 'speaker' when
recovering speaker), remove the unused 'default' member from the type union in
bluetooth-audio-store (the type for availableAudioDevices/selectedAudioDevices),
and update the test fixtures in livekit-bottom-sheet.test.tsx to use
'microphone' and 'speaker' instead of 'default-mic'/'default-speaker' types so
tests match the new role-based types.
src/stores/app/livekit-store.ts
Outdated
| import notifee, { AndroidForegroundServiceType, AndroidImportance } from '@notifee/react-native'; | ||
| import { Audio, InterruptionModeIOS } from 'expo-av'; | ||
| import { getRecordingPermissionsAsync, requestRecordingPermissionsAsync } from 'expo-audio'; | ||
| import { RTCAudioSession } from '@livekit/react-native-webrtc'; |
There was a problem hiding this comment.
Remove unused import: RTCAudioSession.
RTCAudioSession is imported but the code that would use it is commented out (lines 40-48). Remove the unused import.
-import { RTCAudioSession } from '@livekit/react-native-webrtc';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { RTCAudioSession } from '@livekit/react-native-webrtc'; |
🤖 Prompt for AI Agents
In `@src/stores/app/livekit-store.ts` at line 4, Remove the unused import
RTCAudioSession from the top of the file; locate the import statement that reads
"import { RTCAudioSession } from '@livekit/react-native-webrtc'" and delete
RTCAudioSession (or remove the entire import if nothing else is imported from
that module) since the only usage is commented out in the block around the
commented lines referencing RTCAudioSession (around the code that sets audio
session behavior).
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/services/audio.service.ts (1)
68-119: Don’t mark the service initialized if audio loads fail.
preloadAudioAssetsandloadAudioFilesswallow errors, soinitializeAudiostill setsisInitialized = trueand logs success even when sounds didn’t load—this blocks retries and hides failures.✅ Suggested fix
public async preloadAudioAssets(): Promise<void> { try { await Promise.all([ Asset.loadAsync(require('@assets/audio/ui/space_notification1.mp3')), Asset.loadAsync(require('@assets/audio/ui/space_notification2.mp3')), Asset.loadAsync(require('@assets/audio/ui/positive_interface_beep.mp3')), Asset.loadAsync(require('@assets/audio/ui/software_interface_start.mp3')), Asset.loadAsync(require('@assets/audio/ui/software_interface_back.mp3')), ]); logger.debug({ message: 'Audio assets preloaded successfully', }); } catch (error) { logger.error({ message: 'Error preloading audio assets', context: { error }, }); + throw error; } } private async loadAudioFiles(): Promise<void> { try { const { sound: startSound } = await Audio.Sound.createAsync(require('@assets/audio/ui/space_notification1.mp3')); this.startTransmittingSound = startSound; const { sound: stopSound } = await Audio.Sound.createAsync(require('@assets/audio/ui/space_notification2.mp3')); this.stopTransmittingSound = stopSound; const { sound: connectedSound } = await Audio.Sound.createAsync(require('@assets/audio/ui/positive_interface_beep.mp3')); this.connectedDeviceSound = connectedSound; const { sound: connectRoomSound } = await Audio.Sound.createAsync(require('@assets/audio/ui/software_interface_start.mp3')); this.connectToAudioRoomSound = connectRoomSound; const { sound: disconnectRoomSound } = await Audio.Sound.createAsync(require('@assets/audio/ui/software_interface_back.mp3')); this.disconnectedFromAudioRoomSound = disconnectRoomSound; logger.debug({ message: 'Audio files loaded successfully', }); } catch (error) { logger.error({ message: 'Failed to load audio files', context: { error }, }); + throw error; } }
🤖 Fix all issues with AI agents
In `@src/services/callkeep.service.android.ts`:
- Around line 230-275: The setupEventListeners method currently calls
RNCallKeep.addEventListener without storing the returned subscription objects;
change setupEventListeners to save each subscription returned by
RNCallKeep.addEventListener into private fields (e.g., this.endCallSubscription,
this.answerCallSubscription, this.muteSubscription), and update the
cleanup/teardown code to call .remove() on those stored subscriptions (instead
of RNCallKeep.removeEventListener). Ensure references to symbols like
setupEventListeners, currentCallUUID, this.isCallActive, and
this.muteStateCallback remain unchanged while adding the private subscription
fields and invoking .remove() on them during teardown.
In `@src/stores/app/livekit-store.ts`:
- Around line 208-222: The permission request flow calls
PermissionsAndroid.requestMultiple for READ_PHONE_NUMBERS and READ_PHONE_STATE
but does not check the returned result; update the try block in the function
containing this call (the PermissionsAndroid.requestMultiple invocation) to
inspect the granted object for each permission key, and if any are denied or
never_ask_again, log a clear warning via logger.warn (include the denied
permissions and result) and return early or set a flag so downstream CallKeep
usage is skipped; keep the existing catch for thrown errors and ensure you still
log the error as before.
🧹 Nitpick comments (3)
src/stores/app/bluetooth-audio-store.ts (1)
117-165: Consider a factory forINITIAL_STATEto avoid shared references.
INITIAL_STATEcontains nested arrays/objects; exporting and spreading it risks accidental mutations leaking into future resets. A factory keeps fresh copies for store init and reset flows.♻️ Suggested refactor
-export const INITIAL_STATE: Omit< +export const createInitialState = (): Omit< BluetoothAudioState, | 'setBluetoothState' | 'setIsScanning' | 'setIsConnecting' | 'addDevice' | 'updateDevice' | 'removeDevice' | 'clearDevices' | 'setConnectedDevice' | 'setPreferredDevice' | 'setAvailableAudioDevices' | 'setSelectedMicrophone' | 'setSelectedSpeaker' | 'updateAudioDeviceAvailability' | 'setConnectionError' | 'clearConnectionError' | 'setAudioRoutingActive' | 'addButtonEvent' | 'clearButtonEvents' | 'setLastButtonAction' | 'setMediaButtonPTTSettings' | 'setMediaButtonPTTEnabled' -> = { +> => ({ bluetoothState: State.Unknown, isScanning: false, isConnecting: false, availableDevices: [], connectedDevice: null, preferredDevice: null, availableAudioDevices: [ { id: 'default-mic', name: 'Default Microphone', type: 'microphone', isAvailable: true }, { id: 'default-speaker', name: 'Default Speaker', type: 'speaker', isAvailable: true }, ], selectedAudioDevices: { microphone: { id: 'default-mic', name: 'Default Microphone', type: 'microphone', isAvailable: true }, speaker: { id: 'default-speaker', name: 'Default Speaker', type: 'speaker', isAvailable: true }, }, connectionError: null, isAudioRoutingActive: false, buttonEvents: [], lastButtonAction: null, mediaButtonPTTSettings: createDefaultPTTSettings(), -}; +}); + +export const INITIAL_STATE = createInitialState(); export const useBluetoothAudioStore = create<BluetoothAudioState>((set, get) => ({ - ...INITIAL_STATE, + ...createInitialState(),src/stores/app/livekit-store.ts (2)
17-71: Avoid forcing earpiece when routing to Bluetooth/default.
playThroughEarpieceAndroidis set for all non‑speaker routes, which can override Bluetooth routing. Consider only setting it for explicit speaker/earpiece choices and letting Bluetooth/default fall through to system routing.💡 Suggested refinement
- await Audio.setAudioModeAsync({ + const audioMode = { allowsRecordingIOS: true, staysActiveInBackground: true, playsInSilentModeIOS: true, shouldDuckAndroid: true, - // For speaker, we want false (speaker). For others, simple routing. - playThroughEarpieceAndroid: deviceType !== 'speaker', + ...(deviceType === 'speaker' && { playThroughEarpieceAndroid: false }), + ...(deviceType === 'earpiece' && { playThroughEarpieceAndroid: true }), interruptionModeIOS: InterruptionModeIOS.DoNotMix, - }); + }; + await Audio.setAudioModeAsync(audioMode);
79-118: Consider using a true “default” route when nothing is selected.The current default forces
earpiece, which can override the OS default even when the user hasn’t chosen a device. If the intent is to defer to system routing, use'default'and only coerce to speaker/earpiece when explicitly selected.💡 Suggested adjustment
- let targetType: 'bluetooth' | 'speaker' | 'earpiece' = 'earpiece'; + let targetType: 'bluetooth' | 'speaker' | 'earpiece' | 'default' = 'default'; ... - if (selectedAudioDevices.speaker?.type === 'speaker') { + if (selectedAudioDevices.speaker?.type === 'speaker') { targetType = 'speaker'; } + if (selectedAudioDevices.speaker?.type === 'earpiece') { + targetType = 'earpiece'; + }
| /** | ||
| * Setup event listeners for CallKeep events | ||
| */ | ||
| private setupEventListeners(): void { | ||
| // Call ended from CallKit UI | ||
| RNCallKeep.addEventListener('endCall', ({ callUUID }: { callUUID: string }) => { | ||
| logger.info({ | ||
| message: 'CallKeep call ended from system UI', | ||
| context: { callUUID }, | ||
| }); | ||
|
|
||
| if (callUUID === currentCallUUID) { | ||
| currentCallUUID = null; | ||
| this.isCallActive = false; | ||
| } | ||
| }); | ||
|
|
||
| // Call answered (not typically used for outgoing calls, but good to handle) | ||
| RNCallKeep.addEventListener('answerCall', ({ callUUID }: { callUUID: string }) => { | ||
| logger.debug({ | ||
| message: 'CallKeep call answered', | ||
| context: { callUUID }, | ||
| }); | ||
| RNCallKeep.setCurrentCallActive(callUUID); | ||
| }); | ||
|
|
||
| // Mute/unmute events | ||
| RNCallKeep.addEventListener('didPerformSetMutedCallAction', ({ muted, callUUID }: { muted: boolean; callUUID: string }) => { | ||
| logger.debug({ | ||
| message: 'CallKeep mute state changed', | ||
| context: { muted, callUUID }, | ||
| }); | ||
|
|
||
| // Call the registered callback if available | ||
| if (this.muteStateCallback) { | ||
| try { | ||
| this.muteStateCallback(muted); | ||
| } catch (error) { | ||
| logger.warn({ | ||
| message: 'Failed to execute mute state callback', | ||
| context: { error, muted, callUUID }, | ||
| }); | ||
| } | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, locate and read the callkeep service file
if [ -f "src/services/callkeep.service.android.ts" ]; then
echo "=== File found: src/services/callkeep.service.android.ts ==="
wc -l src/services/callkeep.service.android.ts
else
echo "File not found at expected location, searching..."
fd "callkeep.service" --type f
fiRepository: Resgrid/Unit
Length of output: 161
🏁 Script executed:
#!/bin/bash
# Read the setupEventListeners method and surrounding context
cat -n src/services/callkeep.service.android.ts | sed -n '225,315p'Repository: Resgrid/Unit
Length of output: 3277
🏁 Script executed:
#!/bin/bash
# Search for cleanup method to see how event listeners are currently being removed
rg "cleanup|removeEventListener" src/services/callkeep.service.android.ts -A 5 -B 2Repository: Resgrid/Unit
Length of output: 689
🏁 Script executed:
#!/bin/bash
# Check the version of react-native-callkeep
rg "react-native-callkeep" package.jsonRepository: Resgrid/Unit
Length of output: 223
🏁 Script executed:
#!/bin/bash
# Search for react-native-callkeep usage elsewhere in the codebase to see patterns
rg "RNCallKeep\." --type ts --type tsx | head -20Repository: Resgrid/Unit
Length of output: 83
🏁 Script executed:
#!/bin/bash
# Check if there are any type definitions or imports from react-native-callkeep
fd "node_modules/react-native-callkeep" -type d 2>/dev/null | head -1 | xargs -I {} ls -la {} 2>/dev/null | head -20Repository: Resgrid/Unit
Length of output: 38
🌐 Web query:
react-native-callkeep addEventListener return value subscription API
💡 Result:
Web search failed: Server error: no LLM provider could handle the message
🏁 Script executed:
#!/bin/bash
# Try finding type definitions for react-native-callkeep
find . -name "*.d.ts" -type f | xargs grep -l "addEventListener" 2>/dev/null | head -5Repository: Resgrid/Unit
Length of output: 283
🏁 Script executed:
#!/bin/bash
# Check if there's a tsconfig or any type configuration
if [ -f "tsconfig.json" ]; then
cat tsconfig.json | head -30
fiRepository: Resgrid/Unit
Length of output: 797
🏁 Script executed:
#!/bin/bash
# Look for how addEventListener is being used elsewhere - check for subscription pattern
rg "addEventListener" --type ts -A 2 -B 1 | head -40Repository: Resgrid/Unit
Length of output: 3011
🏁 Script executed:
#!/bin/bash
# Check node_modules for the actual react-native-callkeep module structure
if [ -d "node_modules/react-native-callkeep" ]; then
ls -la node_modules/react-native-callkeep/ 2>/dev/null || echo "Directory exists but listing failed"
# Try to find index.d.ts or similar
find node_modules/react-native-callkeep -name "*.d.ts" 2>/dev/null | head -5
fiRepository: Resgrid/Unit
Length of output: 38
🌐 Web query:
react-native-callkeep addEventListener Irfanwani fork API subscription remove
💡 Result:
In the react-native-webrtc/react-native-callkeep API, RNCallKeep.addEventListener(...) is not documented as returning a subscription object. Instead, listeners are removed via CallKeep’s own RNCallKeep.removeEventListener(eventName) API. The README even shows removal like this (no subscription, no handler reference): RNCallKeep.removeEventListener('checkReachability'); [1]
So, for CallKeep, the pattern is typically:
RNCallKeep.addEventListener('answerCall', onAnswerCall);
// ...
RNCallKeep.removeEventListener('answerCall');If your Irfanwani fork changed addEventListener to follow modern React Native EventEmitter conventions (i.e., return an EmitterSubscription), then the removal pattern would be:
const sub = RNCallKeep.addEventListener('answerCall', onAnswerCall);
// ...
sub.remove();That “subscription with .remove()” pattern is how many core RN APIs work today (e.g., BackHandler.addEventListener returns a subscription you remove via subscription.remove()), but it’s not the removal API shown in CallKeep’s upstream README [1], whereas it is shown in React Native’s docs for similar event APIs [2].
Practical check for your fork: log the return value:
const ret = RNCallKeep.addEventListener('answerCall', onAnswerCall);
console.log(ret); // if it has { remove: fn }, use ret.remove()Sources: [1] react-native-callkeep README/API docs, including removeEventListener(...) usage; [2] React Native docs example of addEventListener returning a subscription with .remove().
🏁 Script executed:
#!/bin/bash
# Look for cleanup patterns with addEventListener subscriptions in other services
rg "private.*Sub.*\?\s*:" src/services --type ts -A 1 | head -20Repository: Resgrid/Unit
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Check for any existing subscription storage patterns
rg "addEventListener.*\=" src/services --type ts -B 2 | head -30Repository: Resgrid/Unit
Length of output: 160
🏁 Script executed:
#!/bin/bash
# Check for addEventListener patterns with assignment (fixed regex)
rg "addEventListener.*=" src/services --type ts -B 2 -A 1 | head -40Repository: Resgrid/Unit
Length of output: 2691
🏁 Script executed:
#!/bin/bash
# Look at location.service.ts to see how it handles AppState listener cleanup
rg "AppState.addEventListener\|cleanup" src/services/location.service.ts -B 2 -A 3Repository: Resgrid/Unit
Length of output: 128
🏁 Script executed:
#!/bin/bash
# Find all service files and look for cleanup with event listeners
fd ".*\.service\.ts$" src/services --type f | head -10Repository: Resgrid/Unit
Length of output: 428
🏁 Script executed:
#!/bin/bash
# Check one of the services that had subscription patterns in tests - offline-event-manager
cat -n src/services/offline-event-manager.service.ts | grep -A 15 "private.*Sub\|addEventListener\|cleanup" | head -50Repository: Resgrid/Unit
Length of output: 1676
🏁 Script executed:
#!/bin/bash
# Check the iOS version to see if it has the same issue
grep -n "addEventListener\|removeEventListener\|cleanup" src/services/callkeep.service.ios.ts | tail -20Repository: Resgrid/Unit
Length of output: 929
Store and properly remove event listener subscriptions using the .remove() method.
The addEventListener calls in this service return subscription objects with a .remove() method. The current cleanup implementation uses RNCallKeep.removeEventListener(eventName), which does not match the actual API behavior. Store subscriptions in private fields and call .remove() during cleanup, consistent with the pattern used elsewhere in the codebase (e.g., OfflineEventManager).
🔧 Suggested pattern
+ private endCallSub?: { remove: () => void };
+ private answerCallSub?: { remove: () => void };
+ private muteCallSub?: { remove: () => void };
private setupEventListeners(): void {
- RNCallKeep.addEventListener('endCall', ({ callUUID }: { callUUID: string }) => {
+ this.endCallSub = RNCallKeep.addEventListener('endCall', ({ callUUID }: { callUUID: string }) => {
...
});
- RNCallKeep.addEventListener('answerCall', ({ callUUID }: { callUUID: string }) => {
+ this.answerCallSub = RNCallKeep.addEventListener('answerCall', ({ callUUID }: { callUUID: string }) => {
...
});
- RNCallKeep.addEventListener('didPerformSetMutedCallAction', ({ muted, callUUID }: { muted: boolean; callUUID: string }) => {
+ this.muteCallSub = RNCallKeep.addEventListener('didPerformSetMutedCallAction', ({ muted, callUUID }: { muted: boolean; callUUID: string }) => {
...
});
}
async cleanup(): Promise<void> {
...
- RNCallKeep.removeEventListener('endCall');
- RNCallKeep.removeEventListener('answerCall');
- RNCallKeep.removeEventListener('didPerformSetMutedCallAction');
+ this.endCallSub?.remove();
+ this.answerCallSub?.remove();
+ this.muteCallSub?.remove();
...
}🤖 Prompt for AI Agents
In `@src/services/callkeep.service.android.ts` around lines 230 - 275, The
setupEventListeners method currently calls RNCallKeep.addEventListener without
storing the returned subscription objects; change setupEventListeners to save
each subscription returned by RNCallKeep.addEventListener into private fields
(e.g., this.endCallSubscription, this.answerCallSubscription,
this.muteSubscription), and update the cleanup/teardown code to call .remove()
on those stored subscriptions (instead of RNCallKeep.removeEventListener).
Ensure references to symbols like setupEventListeners, currentCallUUID,
this.isCallActive, and this.muteStateCallback remain unchanged while adding the
private subscription fields and invoking .remove() on them during teardown.
| // Request phone state/numbers permissions for CallKeep (required for Android 11+) | ||
| try { | ||
| // We need these permissions to use the ConnectionService (CallKeep) properly without crashing | ||
| const granted = await PermissionsAndroid.requestMultiple([PermissionsAndroid.PERMISSIONS.READ_PHONE_NUMBERS, PermissionsAndroid.PERMISSIONS.READ_PHONE_STATE]); | ||
|
|
||
| logger.debug({ | ||
| message: 'Android Phone permissions requested', | ||
| context: { result: granted }, | ||
| }); | ||
| } catch (err) { | ||
| logger.warn({ | ||
| message: 'Failed to request Android phone permissions', | ||
| context: { error: err }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and read the livekit-store.ts file
fd -t f "livekit-store.ts" --exec cat -n {} \;Repository: Resgrid/Unit
Length of output: 20192
Handle denied phone permissions proactively in permission request flow.
The result of requestMultiple isn't checked. While CallKeep failures are gracefully handled downstream (lines 349-355), detecting permission denials here would surface the issue earlier to users. Consider checking the result and either warning immediately or returning early if permissions are denied.
🛠️ Suggested handling
- const granted = await PermissionsAndroid.requestMultiple([PermissionsAndroid.PERMISSIONS.READ_PHONE_NUMBERS, PermissionsAndroid.PERMISSIONS.READ_PHONE_STATE]);
+ const granted = await PermissionsAndroid.requestMultiple([
+ PermissionsAndroid.PERMISSIONS.READ_PHONE_NUMBERS,
+ PermissionsAndroid.PERMISSIONS.READ_PHONE_STATE,
+ ]);
+ const denied = Object.entries(granted).filter(
+ ([, status]) => status !== PermissionsAndroid.RESULTS.GRANTED,
+ );
+ if (denied.length) {
+ logger.warn({
+ message: 'Android Phone permissions denied',
+ context: { denied },
+ });
+ return;
+ }📝 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.
| // Request phone state/numbers permissions for CallKeep (required for Android 11+) | |
| try { | |
| // We need these permissions to use the ConnectionService (CallKeep) properly without crashing | |
| const granted = await PermissionsAndroid.requestMultiple([PermissionsAndroid.PERMISSIONS.READ_PHONE_NUMBERS, PermissionsAndroid.PERMISSIONS.READ_PHONE_STATE]); | |
| logger.debug({ | |
| message: 'Android Phone permissions requested', | |
| context: { result: granted }, | |
| }); | |
| } catch (err) { | |
| logger.warn({ | |
| message: 'Failed to request Android phone permissions', | |
| context: { error: err }, | |
| }); | |
| } | |
| // Request phone state/numbers permissions for CallKeep (required for Android 11+) | |
| try { | |
| // We need these permissions to use the ConnectionService (CallKeep) properly without crashing | |
| const granted = await PermissionsAndroid.requestMultiple([ | |
| PermissionsAndroid.PERMISSIONS.READ_PHONE_NUMBERS, | |
| PermissionsAndroid.PERMISSIONS.READ_PHONE_STATE, | |
| ]); | |
| const denied = Object.entries(granted).filter( | |
| ([, status]) => status !== PermissionsAndroid.RESULTS.GRANTED, | |
| ); | |
| if (denied.length) { | |
| logger.warn({ | |
| message: 'Android Phone permissions denied', | |
| context: { denied }, | |
| }); | |
| return; | |
| } | |
| logger.debug({ | |
| message: 'Android Phone permissions requested', | |
| context: { result: granted }, | |
| }); | |
| } catch (err) { | |
| logger.warn({ | |
| message: 'Failed to request Android phone permissions', | |
| context: { error: err }, | |
| }); | |
| } |
🤖 Prompt for AI Agents
In `@src/stores/app/livekit-store.ts` around lines 208 - 222, The permission
request flow calls PermissionsAndroid.requestMultiple for READ_PHONE_NUMBERS and
READ_PHONE_STATE but does not check the returned result; update the try block in
the function containing this call (the PermissionsAndroid.requestMultiple
invocation) to inspect the granted object for each permission key, and if any
are denied or never_ask_again, log a clear warning via logger.warn (include the
denied permissions and result) and return early or set a flag so downstream
CallKeep usage is skipped; keep the existing catch for thrown errors and ensure
you still log the error as before.
This reverts commit f7b8233.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/services/callkeep.service.android.ts`:
- Around line 280-290: The generateUUID() function uses Math.random() fallback
which is not cryptographically secure; import and use the secure randomUUID
implementation from expo-crypto when global.crypto.randomUUID is unavailable:
add an import (e.g. randomUUID from 'expo-crypto') and call it as the fallback
in generateUUID() inside callkeep.service.android.ts (and mirror the same change
in callkeep.service.ios.ts), and update the uuidv4() utility in src/lib/utils.ts
to prefer global.crypto.randomUUID, then expo-crypto.randomUUID, only falling
back to Math.random() as a last resort; reference the generateUUID and uuidv4
function names when making the changes.
In `@src/services/callkeep.service.ios.ts`:
- Around line 218-229: The ignoreMuteEvents function can set ignoreEventsUntil
to NaN when durationMs is non‑finite; add a guard in ignoreMuteEvents to
validate durationMs with Number.isFinite(durationMs) and durationMs > 0, and if
validation fails log a warning via logger.warn (include the bad value) and
return early; otherwise compute const target = Date.now() + durationMs and set
this.ignoreEventsUntil = Math.max(this.ignoreEventsUntil, target) and keep the
existing logger.debug with durationMs and until fields.
🧹 Nitpick comments (10)
src/services/audio.service.ts (5)
2-3: Remove unused imports.
AVPlaybackSourceandPlatformare imported but not used anywhere in this file.🧹 Proposed fix
-import { Audio, type AVPlaybackSource, InterruptionModeIOS } from 'expo-av'; -import { Platform } from 'react-native'; +import { Audio, InterruptionModeIOS } from 'expo-av';
17-17: Remove stale comment.The comment "Keep others as expo-av for now" is incorrect—all sound fields are now typed as
Soundfrom react-native-sound, not expo-av.🧹 Proposed fix
- // Keep others as expo-av for now - private connectedDeviceSound: Sound | null = null; + private connectedDeviceSound: Sound | null = null;
169-187: Misleadingasyncsignature with no awaited operations.The method is marked
asyncbut doesn't await anything—play()uses a callback, not a Promise. Thetry-catchblock only catches synchronous exceptions (e.g., null dereference) but won't catch playback errors reported via the callback. This pattern repeats in all play methods.Consider either:
- Remove
asyncsince there's nothing to await, or- Wrap the callback-based API in a Promise if you need to know when playback completes
♻️ Option 1: Remove async (simpler)
- async playStartTransmittingSound(): Promise<void> { + playStartTransmittingSound(): void { try { if (this.startTransmittingSound) { - // Stop if playing this.startTransmittingSound.stop(); - // Play this.startTransmittingSound.play((success) => { - if (!success) { - logger.warn({ message: 'Failed to play start transmitting sound (playback error)' }); - } + if (!success) { + logger.warn({ message: 'Failed to play start transmitting sound (playback error)' }); + } }); } } catch (error) {
214-219: Inconsistent success logging across play methods.
playConnectedDeviceSoundlogs success at debug level, but other play methods (playStartTransmittingSound,playStopTransmittingSound, etc.) do not. Consider applying consistent logging across all play methods.
16-16: Minor: Trailing whitespace detected by linter.Static analysis flagged trailing whitespace on lines 16, 45, 53, 103, and 128. Consider running your formatter to clean these up.
src/components/ui/select/select-actionsheet.tsx (1)
392-400: Consider default FlashList/FlatList perf props in this wrapper.You’re already setting
estimatedItemSize, but the wrapper doesn’t set or document other perf props (e.g.,removeClippedSubviews,maxToRenderPerBatch,windowSize) or a fixed-size layout hook when rows are uniform. IfActionsheetFlatListhas consistent row heights, consider adding the appropriate layout/perf props (or at least documenting that callers should pass them). Please verify the supported prop names for@shopify/flash-listbefore adding.
As per coding guidelines: "Optimize FlatLists with props likeremoveClippedSubviews,maxToRenderPerBatch, andwindowSize" and "UsegetItemLayoutfor FlatLists when items have a consistent size to improve performance."src/components/ui/actionsheet/index.tsx (1)
392-400: Consider default FlashList/FlatList perf props in this wrapper.This wrapper now standardizes
estimatedItemSize, but it still relies on callers to set windowing/perf props. If list rows are uniform, consider adding or documenting the appropriate perf/layout props (e.g., windowing props or fixed-size layout hooks). Please confirm which of these are supported by@shopify/flash-listbefore wiring them in.
As per coding guidelines: "Optimize FlatLists with props likeremoveClippedSubviews,maxToRenderPerBatch, andwindowSize" and "UsegetItemLayoutfor FlatLists when items have a consistent size to improve performance."src/services/bluetooth-audio.service.ts (1)
1-8: Consider removing the@ts-ignorecomment.The
@ts-ignoresuppresses TypeScript errors for the callkeep service import. If barrel file updates are pending, consider adding an issue to track this technical debt. If the import resolves correctly in the build, the comment may no longer be necessary.src/components/settings/audio-device-selection.tsx (1)
22-33: Consider adding a case for'microphone'type inrenderDeviceIcon.The new
'microphone'type added toAudioDeviceInfofalls through to the defaultMicicon, which is correct but implicit. Consider adding an explicit case for clarity and future maintainability.Optional explicit case
const renderDeviceIcon = (device: AudioDeviceInfo) => { switch (device.type) { case 'bluetooth': return <Headphones size={20} className="text-gray-600 dark:text-gray-400" />; case 'wired': return <Headphones size={20} className="text-gray-600 dark:text-gray-400" />; case 'speaker': return <Speaker size={20} className="text-gray-600 dark:text-gray-400" />; + case 'microphone': + return <Mic size={20} className="text-gray-600 dark:text-gray-400" />; default: return <Mic size={20} className="text-gray-600 dark:text-gray-400" />; } };src/stores/app/bluetooth-audio-store.ts (1)
43-48: Consider removing unused'default'type from the union.With the move to role-based types (
'microphone','speaker'), the'default'type may no longer be used. If no code creates devices withtype: 'default', consider removing it from the union for type safety.#!/bin/bash # Check if 'default' type is still used anywhere rg -n "type:\s*['\"]default['\"]" --type ts
| private generateUUID(): string { | ||
| // RN 0.76 typically provides global crypto.randomUUID via Hermes/JSI | ||
| const rndUUID = (global as any)?.crypto?.randomUUID?.(); | ||
| if (typeof rndUUID === 'string') return rndUUID; | ||
| // Fallback | ||
| return 'xxxxxxxx-xxxx-4xxx-yxxx-xxxxxxxxxxxx'.replace(/[xy]/g, (c) => { | ||
| const r = (Math.random() * 16) | 0; | ||
| const v = c === 'x' ? r : (r & 0x3) | 0x8; | ||
| return v.toString(16); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
expo-crypto randomUUID function API documentation
💡 Result:
expo-crypto exposes Crypto.randomUUID() (synchronous) to generate a UUID v4 (RFC 4122) using cryptographically secure random values.
- Signature:
Crypto.randomUUID(): string - Example:
import * as Crypto from 'expo-crypto'; const id = Crypto.randomUUID();
Official docs: Expo Crypto SDK reference (see the Crypto.randomUUID() section). [1]
Related standard Web API reference (same method name/behavior): MDN Crypto.randomUUID(). [2]
Sources
[1] https://docs.expo.dev/versions/latest/sdk/crypto
[2] https://developer.mozilla.org/en-US/docs/Web/API/Crypto/randomUUID
🏁 Script executed:
# Check if expo-crypto is already used in the project
rg "expo-crypto" --type ts --type tsx --type json
# Check package.json for dependencies
fd package.json | head -5 | xargs catRepository: Resgrid/Unit
Length of output: 11136
🏁 Script executed:
# Read the actual callkeep.service.android.ts file
fd "callkeep.service.android.ts" -type f | head -1 | xargs wc -lRepository: Resgrid/Unit
Length of output: 245
🏁 Script executed:
# Find the callkeep.service.android.ts file
fd "callkeep.service.android.ts" | head -1 | xargs wc -lRepository: Resgrid/Unit
Length of output: 100
🏁 Script executed:
# Read the file to see the generateUUID implementation and imports
cat -n src/services/callkeep.service.android.ts | head -30Repository: Resgrid/Unit
Length of output: 1213
🏁 Script executed:
# Read the generateUUID method around lines 280-290
cat -n src/services/callkeep.service.android.ts | sed -n '275,295p'Repository: Resgrid/Unit
Length of output: 827
🏁 Script executed:
# Check if crypto is used elsewhere in the codebase
rg "crypto" src/ --type ts --type tsx -i
# Check what uuid generation is used elsewhere
rg "uuid\|UUID" src/ --type ts --type tsx -A 2Repository: Resgrid/Unit
Length of output: 111
🏁 Script executed:
# Check if crypto is used elsewhere in the codebase
rg "crypto" src/ -i
# Check what uuid generation is used elsewhere
rg "uuid|UUID" src/ -A 2Repository: Resgrid/Unit
Length of output: 32919
🌐 Web query:
React Native 0.79 Hermes crypto randomUUID availability support
💡 Result:
In React Native 0.79 (where Hermes is the default JS engine), you should not assume globalThis.crypto.randomUUID() is available.
- Hermes does not provide the Web Crypto API surface by default (notably
crypto.getRandomValueshas historically been missing, which is a prerequisite for many UUID/crypto libraries). (github.com) - React Native’s Hermes documentation for 0.79 describes Hermes usage/bundling but does not claim Web Crypto (
crypto.*) compatibility. (reactnative.dev) crypto.randomUUID()is a Web API method (common in browsers), not something guaranteed in Hermes/React Native. (developer.mozilla.org)
What to do instead (common options)
-
Polyfill
crypto.getRandomValues, then use a UUID library (likeuuid) that can run on top of it:react-native-get-random-valuesis specifically intended to polyfillcrypto.getRandomValuesfor React Native. (npm.io)
-
If you need an actual
crypto.randomUUID()API, you typically implement it yourself (e.g., via a native module such asexpo-cryptoin compatible setups) or rely onuuid’s v4 generator after installing thegetRandomValuespolyfill. (stackoverflow.com)
Citations:
- 1: crypto.getRandomValues support (compatibility with uuid 7.x, nanoid, etc.) facebook/hermes#915
- 2: https://reactnative.dev/docs/0.79/hermes?utm_source=openai
- 3: https://developer.mozilla.org/en-US/docs/Web/API/Crypto/randomUUID?utm_source=openai
- 4: https://npm.io/package/react-native-get-random-values?utm_source=openai
- 5: https://stackoverflow.com/questions/66852077/nanoid-cant-be-used-in-react-native?utm_source=openai
🏁 Script executed:
# Check if uuid or react-native-get-random-values are installed
rg "uuid|react-native-get-random-values" package.jsonRepository: Resgrid/Unit
Length of output: 38
🏁 Script executed:
# Check package.json more directly for uuid and related packages
cat package.json | grep -i "uuid\|crypto"Repository: Resgrid/Unit
Length of output: 38
Use cryptographically secure UUID generation for CallKeep sessions.
The Math.random() fallback in generateUUID() is cryptographically weak. React Native 0.79.6 with Hermes does not guarantee crypto.randomUUID() availability, meaning the insecure fallback is likely being used in production to generate call identifiers. Replace with expo-crypto.randomUUID(), which provides secure RFC 4122 UUID generation (also apply the same fix to callkeep.service.ios.ts and the uuidv4() utility function in src/lib/utils.ts).
Proposed fix
+import * as Crypto from 'expo-crypto';
+
private generateUUID(): string {
- // RN 0.76 typically provides global crypto.randomUUID via Hermes/JSI
- const rndUUID = (global as any)?.crypto?.randomUUID?.();
- if (typeof rndUUID === 'string') return rndUUID;
- // Fallback
- return 'xxxxxxxx-xxxx-4xxx-yxxx-xxxxxxxxxxxx'.replace(/[xy]/g, (c) => {
- const r = (Math.random() * 16) | 0;
- const v = c === 'x' ? r : (r & 0x3) | 0x8;
- return v.toString(16);
- });
+ return Crypto.randomUUID();
}🤖 Prompt for AI Agents
In `@src/services/callkeep.service.android.ts` around lines 280 - 290, The
generateUUID() function uses Math.random() fallback which is not
cryptographically secure; import and use the secure randomUUID implementation
from expo-crypto when global.crypto.randomUUID is unavailable: add an import
(e.g. randomUUID from 'expo-crypto') and call it as the fallback in
generateUUID() inside callkeep.service.android.ts (and mirror the same change in
callkeep.service.ios.ts), and update the uuidv4() utility in src/lib/utils.ts to
prefer global.crypto.randomUUID, then expo-crypto.randomUUID, only falling back
to Math.random() as a last resort; reference the generateUUID and uuidv4
function names when making the changes.
| /** | ||
| * Externally lock/ignore mute events for a duration. | ||
| * Useful when we know a PTT button is being pressed and want to ignore system side-effects. | ||
| */ | ||
| ignoreMuteEvents(durationMs: number): void { | ||
| const now = Date.now(); | ||
| this.ignoreEventsUntil = Math.max(this.ignoreEventsUntil, now + durationMs); | ||
| logger.debug({ | ||
| message: 'CallKeep mute events ignored via external lock', | ||
| context: { durationMs, until: this.ignoreEventsUntil } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Guard against non‑finite or non‑positive durations.
If durationMs is NaN, Math.max yields NaN, which permanently disables the lock check (now < NaN is always false). A simple validation avoids this edge case and misleading logs.
✅ Proposed fix
ignoreMuteEvents(durationMs: number): void {
+ if (!Number.isFinite(durationMs) || durationMs <= 0) {
+ logger.debug({
+ message: 'CallKeep ignoreMuteEvents no-op (invalid duration)',
+ context: { durationMs },
+ });
+ return;
+ }
const now = Date.now();
this.ignoreEventsUntil = Math.max(this.ignoreEventsUntil, now + durationMs);
logger.debug({
message: 'CallKeep mute events ignored via external lock',
context: { durationMs, until: this.ignoreEventsUntil }
});
}🤖 Prompt for AI Agents
In `@src/services/callkeep.service.ios.ts` around lines 218 - 229, The
ignoreMuteEvents function can set ignoreEventsUntil to NaN when durationMs is
non‑finite; add a guard in ignoreMuteEvents to validate durationMs with
Number.isFinite(durationMs) and durationMs > 0, and if validation fails log a
warning via logger.warn (include the bad value) and return early; otherwise
compute const target = Date.now() + durationMs and set this.ignoreEventsUntil =
Math.max(this.ignoreEventsUntil, target) and keep the existing logger.debug with
durationMs and until fields.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/components/ui/actionsheet/index.tsx`:
- Around line 392-398: The ActionsheetFlatList currently destructures and
applies the prop named "style" only to the wrapper View, preventing consumers
from styling the inner UIActionsheet.FlatList; update the component
(ActionsheetFlatList) so the FlatList also receives styling (either by
forwarding the existing "style" prop into UIActionsheet.FlatList as well or by
adding a new prop like "flatListStyle" or "listStyle" that is applied to
UIActionsheet.FlatList while keeping "style" for the wrapper View), and ensure
to forward any FlashList-specific style props (e.g., scroll indicator styles) to
UIActionsheet.FlatList so existing consumers aren’t broken.
In `@src/components/ui/select/select-actionsheet.tsx`:
- Around line 392-398: The wrapper View in ActionsheetFlatList is consuming the
incoming style prop instead of forwarding it to the inner
UIActionsheet.FlatList; update ActionsheetFlatList so the container uses a
distinct prop (e.g., containerStyle or wrapperStyle) and forward the original
style prop to UIActionsheet.FlatList (UIActionsheet.FlatList ref and props) so
consumers can style the FlashList directly; apply the same pattern you used to
fix src/components/ui/actionsheet/index.tsx to keep API consistency.
In `@src/services/audio.service.ts`:
- Around line 55-59: Sound mappings are inconsistent between the native module
calls (InCallAudioModule.loadSound for keys startTransmitting, stopTransmitting,
connectToAudioRoom, disconnectedFromAudioRoom) and the Expo AV fallback; update
the Expo AV loading logic so the same sound keys map to the identical asset
names used by the native module (i.e., ensure
startTransmitting→software_interface_start,
stopTransmitting→software_interface_back,
connectToAudioRoom→space_notification1,
disconnectedFromAudioRoom→space_notification2) by changing the Expo AV mapping
block to use those exact asset identifiers where the Expo loader registers those
four keys.
In `@src/translations/ar.json`:
- Line 50: The "device_disconnected" translation value is still in English;
update the JSON key "device_disconnected" in translations/ar.json to its Arabic
translation (for example: "تم فصل الجهاز") so the value is localized
consistently with the rest of the file; ensure the string is enclosed in double
quotes and the trailing comma/JSON formatting remains valid.
🧹 Nitpick comments (8)
src/app/login/login-form.tsx (2)
77-81: User-facing text should be wrapped int()for i18n support.Several hardcoded strings are not internationalized:
- Line 77:
"Sign In"- Lines 79-81: The descriptive paragraph
- Line 118:
"Username not found"- Line 172:
"Log in"As per coding guidelines, all user-facing text should be wrapped in
t()fromreact-i18next.🌐 Proposed fix for internationalization
- <Text className="pb-6 text-center text-4xl font-bold">Sign In</Text> + <Text className="pb-6 text-center text-4xl font-bold">{t('login.sign_in')}</Text> - <Text className="mb-6 max-w-xl text-center text-gray-500"> - To login in to the Resgrid Unit app, please enter your username and password. Resgrid Unit is an app designed to interface between a Unit (apparatus, team, etc) and the Resgrid system. - </Text> + <Text className="mb-6 max-w-xl text-center text-gray-500"> + {t('login.description')} + </Text>Similarly for other hardcoded strings.
96-98: Avoid usinganytype in catch blocks.The
error: anytype annotation violates the guideline to avoidanyand strive for precise types. Consider usingunknownand narrowing the type, or using Zod's error type.🔧 Proposed fix
- } catch (error: any) { - return error.message; + } catch (error: unknown) { + if (error instanceof z.ZodError) { + return error.errors[0]?.message; + } + return 'Validation failed'; }Apply the same fix to the password validation block at lines 135-137.
src/components/ui/select/select-actionsheet.tsx (1)
243-246: Consider documenting the style prop's new behavior in the type.The
styleprop fromReact.ComponentProps<typeof UIActionsheet.FlatList>now applies to the wrapper View, not the FlatList itself. Adding a JSDoc comment on the type or renaming the prop would help clarify this for consumers.type IActionsheetFlatListProps = VariantProps<typeof actionsheetFlatListStyle> & - React.ComponentProps<typeof UIActionsheet.FlatList> & { + Omit<React.ComponentProps<typeof UIActionsheet.FlatList>, 'style'> & { className?: string; + /** Style applied to the wrapper View container, not the inner FlatList */ + style?: ViewStyle; };plugins/withInCallAudioModule.js (1)
236-252: Package registration logic may miss edge cases.The conditional logic for adding
InCallAudioPackageassumes specific patterns inMainApplication.kt. If another plugin has modified the packages declaration differently, this may not insert correctly.Consider adding a fallback or verification:
🛠️ Suggested improvement
if (packagesMatch) { // Using the simplest replacement that ensures toMutableList() const replacement = `val packages = PackageList(this).packages.toMutableList()\n packages.add(InCallAudioPackage())`; // Avoid double adding if MediaButtonPackage logic already changed it to mutable if (mainApplication.contents.includes('packages.add(MediaButtonPackage()')) { // Add ours after MediaButtonPackage mainApplication.contents = mainApplication.contents.replace('packages.add(MediaButtonPackage())', 'packages.add(MediaButtonPackage())\n packages.add(InCallAudioPackage())'); } else { // Standard replacement mainApplication.contents = mainApplication.contents.replace(packagesPattern, replacement); } console.log('[withInCallAudioModule] Registered InCallAudioPackage in MainApplication.kt'); + } else { + console.warn('[withInCallAudioModule] Could not find packages declaration pattern in MainApplication.kt - manual registration may be required'); }src/services/audio.service.ts (1)
103-103: Avoid usinganyfor the module parameter.The
moduleparameter usesanytype. Consider using a more specific type.- private async loadSound(module: any): Promise<Audio.Sound | null> { + private async loadSound(module: number): Promise<Audio.Sound | null> {The
require()call returns anumber(asset ID) when using Metro bundler with assets.src/components/settings/bluetooth-device-selection-bottom-sheet.tsx (2)
63-83: Remove commented-out code block.This large block of commented code explaining the previous implementation should be removed to keep the codebase clean. The git history preserves the previous approach if needed.
Proposed fix
async (device: BluetoothAudioDevice) => { try { - // Disconnect from any currently connected device first? - // The service connectToDevice usually handles this or we do it here. - // Previous code did disconnect manually. - /* - if (connectedDevice) { - ... disconnect ... - } - */ - // User wants "When attempting to connect... display loading". - // User wants "If error... don't save device". - - // 1. Set connecting state - // We can resolve this by using local state for the specific device being connected to, - // or rely on store's global isConnecting. - // Let's use the store's setIsConnecting to be consistent if the UI uses it. useBluetoothAudioStore.getState().setIsConnecting(true);
119-121: Wrap fallback strings in t() for consistency.The fallback strings
'Connection Failed'and'Could not connect to device'should be wrapped int()to maintain i18n consistency across the application.Proposed fix
showMessage({ - message: t('bluetooth.connection_error_title') || 'Connection Failed', - description: errorMessage === 'Device disconnected' ? t('bluetooth.device_disconnected') : errorMessage || t('bluetooth.connection_error_message') || 'Could not connect to device', + message: t('bluetooth.connection_error_title'), + description: errorMessage === 'Device disconnected' ? t('bluetooth.device_disconnected') : errorMessage || t('bluetooth.connection_error_message'), type: 'danger', duration: 4000, });As per coding guidelines: Wrap all user-facing text in t() from react-i18next.
src/services/callkeep.service.android.ts (1)
209-214: Consider implementing mute event suppression for Android.The
ignoreMuteEventsmethod is currently a no-op on Android. If the iOS implementation provides storm-protection for mute events, consider whether Android would benefit from similar behavior for consistency, or document why it's not needed.
| const ActionsheetFlatList = React.forwardRef<React.ElementRef<typeof UIActionsheet.FlatList>, IActionsheetFlatListProps>(({ className, style, ...props }, ref) => { | ||
| return ( | ||
| <UIActionsheet.FlatList | ||
| className={actionsheetFlatListStyle({ | ||
| class: className, | ||
| })} | ||
| ref={ref} | ||
| {...props} | ||
| /> | ||
| <View className={actionsheetFlatListStyle({ class: className })} style={style}> | ||
| <UIActionsheet.FlatList ref={ref} estimatedItemSize={94} {...props} /> | ||
| </View> | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Breaking change: style prop now applies to wrapper View, not the FlatList.
The style prop is destructured and applied to the wrapper View (line 394), meaning consumers expecting to style the underlying FlashList directly can no longer do so. Consider either:
- Passing
stylethrough to the inner FlatList as well, or - Adding a separate prop (e.g.,
listStyleorflatListStyle) for styling the inner FlatList, or - Documenting this as a breaking change in the PR.
Additionally, FlashList-specific style props like scroll indicator styling may not reach the inner component as expected.
🔧 Suggested approach to preserve both wrapper and inner styling
-const ActionsheetFlatList = React.forwardRef<React.ElementRef<typeof UIActionsheet.FlatList>, IActionsheetFlatListProps>(({ className, style, ...props }, ref) => {
+const ActionsheetFlatList = React.forwardRef<React.ElementRef<typeof UIActionsheet.FlatList>, IActionsheetFlatListProps & { wrapperStyle?: ViewStyle }>(({ className, style, wrapperStyle, ...props }, ref) => {
return (
- <View className={actionsheetFlatListStyle({ class: className })} style={style}>
- <UIActionsheet.FlatList ref={ref} estimatedItemSize={94} {...props} />
+ <View className={actionsheetFlatListStyle({ class: className })} style={wrapperStyle}>
+ <UIActionsheet.FlatList ref={ref} estimatedItemSize={94} style={style} {...props} />
</View>
);
});🤖 Prompt for AI Agents
In `@src/components/ui/actionsheet/index.tsx` around lines 392 - 398, The
ActionsheetFlatList currently destructures and applies the prop named "style"
only to the wrapper View, preventing consumers from styling the inner
UIActionsheet.FlatList; update the component (ActionsheetFlatList) so the
FlatList also receives styling (either by forwarding the existing "style" prop
into UIActionsheet.FlatList as well or by adding a new prop like "flatListStyle"
or "listStyle" that is applied to UIActionsheet.FlatList while keeping "style"
for the wrapper View), and ensure to forward any FlashList-specific style props
(e.g., scroll indicator styles) to UIActionsheet.FlatList so existing consumers
aren’t broken.
| const ActionsheetFlatList = React.forwardRef<React.ComponentRef<typeof UIActionsheet.FlatList>, IActionsheetFlatListProps>(function ActionsheetFlatList({ className, style, ...props }, ref) { | ||
| return ( | ||
| <UIActionsheet.FlatList | ||
| className={actionsheetFlatListStyle({ | ||
| class: className, | ||
| })} | ||
| ref={ref} | ||
| {...props} | ||
| /> | ||
| <View className={actionsheetFlatListStyle({ class: className })} style={style}> | ||
| <UIActionsheet.FlatList ref={ref} estimatedItemSize={94} {...props} /> | ||
| </View> | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Same style prop behavior concern as src/components/ui/actionsheet/index.tsx.
This implementation mirrors the changes in actionsheet/index.tsx, which is good for API consistency. However, the same concern applies: style is now consumed by the wrapper View rather than being forwarded to the inner FlashList.
Consider applying the same fix pattern to both files if you decide to preserve direct FlashList styling capability.
🤖 Prompt for AI Agents
In `@src/components/ui/select/select-actionsheet.tsx` around lines 392 - 398, The
wrapper View in ActionsheetFlatList is consuming the incoming style prop instead
of forwarding it to the inner UIActionsheet.FlatList; update ActionsheetFlatList
so the container uses a distinct prop (e.g., containerStyle or wrapperStyle) and
forward the original style prop to UIActionsheet.FlatList
(UIActionsheet.FlatList ref and props) so consumers can style the FlashList
directly; apply the same pattern you used to fix
src/components/ui/actionsheet/index.tsx to keep API consistency.
| InCallAudioModule.loadSound('startTransmitting', 'software_interface_start'); | ||
| InCallAudioModule.loadSound('stopTransmitting', 'software_interface_back'); | ||
| InCallAudioModule.loadSound('connectedDevice', 'positive_interface_beep'); | ||
| InCallAudioModule.loadSound('connectToAudioRoom', 'space_notification1'); | ||
| InCallAudioModule.loadSound('disconnectedFromAudioRoom', 'space_notification2'); |
There was a problem hiding this comment.
Sound file mapping inconsistency between native module and Expo AV.
The sound files are mapped differently between the Android native module (lines 55-59) and the Expo AV fallback (lines 118-124):
| Sound Name | Native Module | Expo AV |
|---|---|---|
| startTransmitting | software_interface_start | space_notification1 |
| stopTransmitting | software_interface_back | space_notification2 |
| connectToAudioRoom | space_notification1 | software_interface_start |
| disconnectedFromAudioRoom | space_notification2 | software_interface_back |
This means Android users will hear different sounds than iOS users for the same actions.
🔧 Proposed fix - align Expo AV with native module mapping
const [startTransmittingSound, stopTransmittingSound, connectedDeviceSound, connectToAudioRoomSound, disconnectedFromAudioRoomSound] = await Promise.all([
- this.loadSound(require('@assets/audio/ui/space_notification1.mp3')),
- this.loadSound(require('@assets/audio/ui/space_notification2.mp3')),
+ this.loadSound(require('@assets/audio/ui/software_interface_start.mp3')),
+ this.loadSound(require('@assets/audio/ui/software_interface_back.mp3')),
this.loadSound(require('@assets/audio/ui/positive_interface_beep.mp3')),
- this.loadSound(require('@assets/audio/ui/software_interface_start.mp3')),
- this.loadSound(require('@assets/audio/ui/software_interface_back.mp3')),
+ this.loadSound(require('@assets/audio/ui/space_notification1.mp3')),
+ this.loadSound(require('@assets/audio/ui/space_notification2.mp3')),
]);Also applies to: 118-124
🤖 Prompt for AI Agents
In `@src/services/audio.service.ts` around lines 55 - 59, Sound mappings are
inconsistent between the native module calls (InCallAudioModule.loadSound for
keys startTransmitting, stopTransmitting, connectToAudioRoom,
disconnectedFromAudioRoom) and the Expo AV fallback; update the Expo AV loading
logic so the same sound keys map to the identical asset names used by the native
module (i.e., ensure startTransmitting→software_interface_start,
stopTransmitting→software_interface_back,
connectToAudioRoom→space_notification1,
disconnectedFromAudioRoom→space_notification2) by changing the Expo AV mapping
block to use those exact asset identifiers where the Expo loader registers those
four keys.
| "connected": "متصل", | ||
| "connectionError": "خطأ في الاتصال", | ||
| "current_selection": "الاختيار الحالي", | ||
| "device_disconnected": "Device disconnected", |
There was a problem hiding this comment.
Missing Arabic translation.
The value "Device disconnected" is in English instead of Arabic. Based on the translation patterns in this file, the Arabic translation should be:
- "device_disconnected": "Device disconnected",
+ "device_disconnected": "تم قطع اتصال الجهاز",📝 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.
| "device_disconnected": "Device disconnected", | |
| "device_disconnected": "تم قطع اتصال الجهاز", |
🤖 Prompt for AI Agents
In `@src/translations/ar.json` at line 50, The "device_disconnected" translation
value is still in English; update the JSON key "device_disconnected" in
translations/ar.json to its Arabic translation (for example: "تم فصل الجهاز") so
the value is localized consistently with the rest of the file; ensure the string
is enclosed in double quotes and the trailing comma/JSON formatting remains
valid.
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.