Develop #89
Develop #89
Conversation
📝 WalkthroughWalkthroughUpdates add CI steps to set Electron package version, replace an external is-dev check with Electron's app.isPackaged, change productName, remove audio-stream UI and onOpenAudioStreams prop, implement auto-connect and audio feedback for PTT, and add web microphone permission handling and "disconnected" translations. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PTTInterface
participant LiveKitStore
participant AudioService
participant LiveKit
User->>PTTInterface: Press PTT
PTTInterface->>PTTInterface: Check isConnected
alt Not connected
PTTInterface->>PTTInterface: Select default/first channel
PTTInterface->>LiveKitStore: requestPermissions()
LiveKitStore->>LiveKitStore: navigator.mediaDevices.getUserMedia(audio)
LiveKitStore-->>PTTInterface: Permission result
PTTInterface->>LiveKitStore: connectToRoom()
LiveKit-->>LiveKitStore: Connected
end
PTTInterface->>PTTInterface: Enable microphone
PTTInterface->>AudioService: playStartTransmittingSound()
AudioService-->>User: Play start sound
User->>PTTInterface: Release PTT
PTTInterface->>PTTInterface: Disable microphone
PTTInterface->>AudioService: playStopTransmittingSound()
AudioService-->>User: Play stop sound
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/react-native-cicd.yml (1)
209-218:⚠️ Potential issue | 🟠 MajorAlign package.json version to full semver (MAJOR.MINOR.PATCH).
The current code writes
1.${{ github.run_number }}(2 segments) topackage.json, which violates semver requirements enforced by npm, EAS/Expo, and Electron Builder. npm requires versions parseable by node-semver (3-part core). EAS'sautoIncrement: "version"expects MAJOR.MINOR.PATCH. Electron Builder rejects non-SemVer formats with "App version is not valid semver". Update to1.${{ github.run_number }}.0(3 segments) to match the pattern already used forELECTRON_VERSIONelsewhere in the workflow and prevent build failures.🔧 Suggested fix
- jq --arg version "1.${{ github.run_number }}" --argjson versionCode "$androidVersionCode" '.version = $version | .versionCode = $versionCode' package.json > package.json.tmp && mv package.json.tmp package.json + jq --arg version "1.${{ github.run_number }}.0" --argjson versionCode "$androidVersionCode" '.version = $version | .versionCode = $versionCode' package.json > package.json.tmp && mv package.json.tmp package.jsonsrc/components/dispatch-console/ptt-interface.tsx (1)
27-80:⚠️ Potential issue | 🟡 MinorWrap the hard-coded 'Disconnected' default in
t()for proper internationalization.The parameter default
currentChannel: externalChannel = 'Disconnected'bypasses translation whenisConnectedis true butpttChannel?.Nameis falsy. Remove the hard-coded default and use a translated fallback instead.Proposed fix
-export const PTTInterface: React.FC<PTTInterfaceProps> = ({ onPTTPress, onPTTRelease, isTransmitting: externalTransmitting = false, currentChannel: externalChannel = 'Disconnected' }) => { +export const PTTInterface: React.FC<PTTInterfaceProps> = ({ onPTTPress, onPTTRelease, isTransmitting: externalTransmitting = false, currentChannel: externalChannel }) => { const { t } = useTranslation(); const { colorScheme } = useColorScheme(); @@ - const displayChannel = isConnected ? (pttChannel?.Name || externalChannel) : (pttChannel?.Name || t('dispatch.disconnected')); + const fallbackChannel = externalChannel ?? t('dispatch.disconnected'); + const displayChannel = isConnected ? (pttChannel?.Name || fallbackChannel) : (pttChannel?.Name || t('dispatch.disconnected'));
🤖 Fix all issues with AI agents
In `@src/hooks/use-ptt.ts`:
- Around line 388-401: The catch path currently treats failures from
audioService.playStartTransmittingSound() as a transmit error; change this to
make SFX playback best-effort by isolating or wrapping
playStartTransmittingSound() in its own try/catch so playback errors are logged
(e.g., logger.warn) but do not set or report transmit failure or revert the mic
state; apply the same pattern for audioService.playStopTransmittingSound() (the
similar block around lines 425-427) so audio playbacks cannot abort or mark the
transmit flow as failed.
In `@src/stores/app/livekit-store.ts`:
- Around line 124-129: The Biome rule flags the implicit return from the arrow
used in stream.getTracks().forEach((track) => track.stop()); — update the
forEach callback to use a block-bodied arrow function so it does not return a
value (e.g., replace the single-expression arrow with a block that calls
track.stop();). Locate this code in the navigator.mediaDevices.getUserMedia try
block in livekit-store.ts and change the callback for stream.getTracks().forEach
accordingly.
🧹 Nitpick comments (1)
src/stores/app/livekit-store.ts (1)
134-153: Avoidanyin permission error handling.
Useunknownand narrow the shape to keep type safety.As per coding guidelines: Avoid using `any`; strive for precise types.🔧 Proposed refactor
- } catch (mediaError: any) { - if (mediaError.name === 'NotAllowedError') { + } catch (mediaError: unknown) { + const errorName = + typeof mediaError === 'object' && mediaError !== null && 'name' in mediaError + ? String((mediaError as { name: unknown }).name) + : 'UnknownError'; + if (errorName === 'NotAllowedError') { logger.error({ message: 'Microphone permission denied by user', context: { platform: 'web', error: mediaError }, }); // Only throw on permission denied - this requires user action throw mediaError; - } else if (mediaError.name === 'NotFoundError') { + } else if (errorName === 'NotFoundError') { // No microphone found - log warning but continue // User can still listen and may connect a microphone later logger.warn({ message: 'No microphone device found - voice channel will be listen-only until a microphone is connected', context: { platform: 'web' }, });
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/stores/app/livekit-store.ts (1)
136-198:⚠️ Potential issue | 🟠 MajorOn web,
NotAllowedErroris swallowed by the outer catch—connection proceeds despite explicit mic permission denial.The web flow throws
NotAllowedErrorwhen the user denies permission (line 141), but the outer catch block (line 184) logs the error without rethrowing. This allowsconnectToRoomto continue and create a room connection. Android/iOS correctly return early on denial (line 160), but the web platform lacks this guard. Rethrow the error in the outer catch for web permission denials, or better yet, let the error propagate naturally by not catching it for this specific case.Also, the
mediaErrorparameter is typed asany(line 137), which violates the TypeScript guideline to avoidanytypes. Use a more specific type likeDOMException.src/stores/app/__tests__/livekit-store.test.ts (1)
114-121:⚠️ Potential issue | 🟠 MajorUpdate web-platform test expectations for new warning paths.
requestPermissionsnow logs warnings on web whennavigator.mediaDevicesis unavailable orgetUserMediafails, so the "Unsupported platform handling" test expectations are outdated. Mocknavigator.mediaDevicesand assert the expected warn calls instead of no logging.🧪 Suggested test update
describe('Unsupported platform handling', () => { beforeEach(() => { (Platform as any).OS = 'web'; + (global as any).navigator = { + mediaDevices: { + getUserMedia: jest.fn().mockRejectedValue({ name: 'NotFoundError' }), + }, + }; }); it('should handle unsupported platform gracefully', async () => { const { requestPermissions } = useLiveKitStore.getState(); await requestPermissions(); expect(mockGetRecordingPermissionsAsync).not.toHaveBeenCalled(); expect(mockRequestRecordingPermissionsAsync).not.toHaveBeenCalled(); - // For unsupported platforms, the function just returns without logging - expect(mockLogger.info).not.toHaveBeenCalled(); - expect(mockLogger.error).not.toHaveBeenCalled(); + expect(mockLogger.warn).toHaveBeenCalled(); }); });
🧹 Nitpick comments (1)
src/stores/app/livekit-store.ts (1)
122-156: Guardnavigatorand avoidanyin the web permission path.
In non-browser contexts (tests/SSR),navigatorcan be undefined, andanydefeats type-safety.♻️ Proposed adjustment
- if (navigator.mediaDevices && navigator.mediaDevices.getUserMedia) { + const mediaDevices = typeof navigator !== 'undefined' ? navigator.mediaDevices : undefined; + if (mediaDevices?.getUserMedia) { try { // Request microphone access - this will prompt the user for permission - const stream = await navigator.mediaDevices.getUserMedia({ audio: true }); + const stream = await mediaDevices.getUserMedia({ audio: true }); // Release the stream immediately - LiveKit will get its own stream stream.getTracks().forEach((track) => { track.stop(); }); logger.info({ message: 'Microphone permission granted successfully', context: { platform: 'web' }, }); - } catch (mediaError: any) { - if (mediaError.name === 'NotAllowedError') { + } catch (mediaError: unknown) { + const errorName = + typeof mediaError === 'object' && mediaError !== null && 'name' in mediaError + ? String((mediaError as { name?: unknown }).name) + : undefined; + if (errorName === 'NotAllowedError') { logger.error({ message: 'Microphone permission denied by user', context: { platform: 'web', error: mediaError }, }); // Only throw on permission denied - this requires user action throw mediaError; - } else if (mediaError.name === 'NotFoundError') { + } else if (errorName === 'NotFoundError') { // No microphone found - log warning but continue // User can still listen and may connect a microphone later logger.warn({ message: 'No microphone device found - voice channel will be listen-only until a microphone is connected', context: { platform: 'web' }, }); } else { logger.warn({ message: 'Failed to request microphone permission - continuing with limited audio', context: { platform: 'web', error: mediaError }, }); } }As per coding guidelines, avoid using
any; strive for precise types.
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores
Translations
Tests