RU-T47 Build version number fix #210
Conversation
📝 WalkthroughWalkthroughThis PR removes MediaButtonService and replaces it with CallKeep-based mute synchronization. It adds system audio device support to Bluetooth connections, introduces device type classification (specialized vs. system), and implements microphone control delegation to the LiveKit store with proper lifecycle management. Changes
Sequence DiagramsequenceDiagram
participant User
participant UI as Bluetooth Device Sheet
participant BtAudio as BluetoothAudioService
participant CK as CallKeepService
participant LK as LiveKitStore
participant Audio as Audio System
User->>UI: Select System Audio
UI->>BtAudio: connectToSystemAudio()
BtAudio->>BtAudio: Disconnect current device
BtAudio->>CK: restoreMuteListener()
CK->>Audio: Re-register mute listener
BtAudio->>LK: Revert audio routing
BtAudio->>LK: Set preferred device (system-audio)
LK->>Audio: Configure microphone routing
BtAudio->>UI: Close sheet on success
Note over CK,LK: Mute synchronization via CallKeep
User->>Audio: Press mute button
CK->>CK: Guard checks (debounce, spam)
CK->>LK: Trigger muteStateCallback
LK->>LK: setMicrophoneEnabled(toggled)
LK->>Audio: Update microphone state
LK->>Audio: Play audio cue
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ 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 (1)
src/services/callkeep.service.ios.ts (1)
280-288:⚠️ Potential issue | 🟡 MinorConsider invoking
endCallCallbackonly for the current callThe
endCallCallbackis invoked regardless of whethercallUUIDmatchescurrentCallUUID. This could cause unintended behavior if there are multiple calls or stale events. Consider guarding the callback invocation.🛡️ Proposed fix
if (callUUID === currentCallUUID) { currentCallUUID = null; this.isCallActive = false; - } - if (this.endCallCallback) { - this.endCallCallback(); + if (this.endCallCallback) { + this.endCallCallback(); + } }
🤖 Fix all issues with AI agents
In @.agent/rules/rules.md:
- Line 44: Fix the spelling mistake in the rule text: replace the word
"dictonary" with "dictionary" in the sentence that mentions storing translation
files in src/translations (the line containing "The app is multi-lingual, so
ensure all text is wrapped in `t()` from `react-i18next` for translations with
the dictonary files stored in `src/translations`.").
In `@src/components/settings/bluetooth-device-selection-bottom-sheet.tsx`:
- Around line 276-279: In the BluetoothDeviceSelectionBottomSheet component,
replace the hardcoded user-facing strings in the two <Text> elements ("System
Audio" and "AirPods, Car, Wired Headset") with calls to the i18n translator
(t()) from react-i18next (ensure t is imported/available in the component), e.g.
t('settings.systemAudio') and t('settings.systemAudioExamples'), and
update/create corresponding keys in the translations files; also ensure the JSX
formatting uses single-line expressions or proper multilines to satisfy the
static analyzer (keep each <Text> content as a single JS expression calling
t()).
🧹 Nitpick comments (11)
src/services/callkeep.service.android.ts (1)
224-236: No-op stubs for interface compatibility.These methods provide API parity with the iOS implementation. Consider adding a brief inline comment explaining they are Android no-ops for cross-platform compatibility.
src/stores/app/bluetooth-audio-store.ts (1)
33-37: Inconsistent indentation in interface definition.The
BluetoothSystemAudioDeviceinterface uses 5-space indentation instead of the 2-space convention used elsewhere in the file.🔧 Proposed fix
export interface BluetoothSystemAudioDevice { - id: string; - name: string; - type: 'system'; + id: string; + name: string; + type: 'system'; }src/services/__tests__/bluetooth-audio.service.test.ts (1)
174-197: Test coverage for microphone control delegation looks good.The tests verify that the bluetooth audio service correctly delegates microphone control to the LiveKit store. Consider extracting the mock retrieval to a constant outside the test body for cleaner access:
💡 Alternative mock access pattern
// At the top of the describe block or test file const getLiveKitStoreMock = () => require('@/stores/app/livekit-store').useLiveKitStore.getState(); // Then in tests: expect(getLiveKitStoreMock().toggleMicrophone).toHaveBeenCalled();src/stores/app/__tests__/livekit-store.test.ts (1)
46-46: Minor: Trailing empty line from removed mock.This empty line appears to be a remnant from removing the media-button service mock. Consider removing it for cleaner formatting.
.agent/rules/rules.md (1)
42-43: Minor grammar improvement suggestionThe sentence structure could be cleaner.
-- Create and use Jest to test to validate all generated components -- Generate tests for all components, services and logic generated. Ensure tests run without errors and fix any issues. +- Use Jest to validate all generated components +- Generate tests for all components, services, and logic. Ensure tests run without errors and resolve any issues.src/stores/app/livekit-store.ts (3)
378-390: Dead code:onLocalTrackMutedis defined but never usedThe
onLocalTrackMutedfunction is declared but never attached to any listener or invoked. Either remove it or implement the intended functionality.- const onLocalTrackMuted = () => { - // Update CallKeep state if needed? - // For now, let's just trust CallKeep's own state management or system UI. - }; - - // We attach these listeners to the local participant if needed for other UI sync
342-367: Simplify inline comments in mute sync callbackThe verbose inline comments explaining the logic can be reduced. The condition check and action are correct, but the explanation is overly detailed for production code.
♻️ Suggested simplification
// Setup CallKeep mute sync callKeepService.setMuteStateCallback(async (muted) => { logger.info({ message: 'Syncing mute state from CallKeep', context: { muted } }); - if (room.localParticipant.isMicrophoneEnabled === muted) { - // If CallKeep says "muted" (true), and Mic is enabled (true), we need to disable mic. - // If CallKeep says "unmuted" (false), and Mic is disabled (false), we need to enable mic. - // Wait, logic check: - // isMicrophoneEnabled = true means NOT MUTED. - // muted = true means MUTED. - // So if isMicrophoneEnabled (true) and muted (true) -> mismatch, we must mute. - // if isMicrophoneEnabled (false) and muted (false) -> mismatch, we must unmute. - - // Actually effectively: setMicrophoneEnabled(!muted) - await room.localParticipant.setMicrophoneEnabled(!muted); + // Sync mic state: isMicrophoneEnabled should be the inverse of muted + const shouldBeEnabled = !muted; + if (room.localParticipant.isMicrophoneEnabled !== shouldBeEnabled) { + await room.localParticipant.setMicrophoneEnabled(shouldBeEnabled); // Play sound if (!muted) { await audioService.playStartTransmittingSound(); } else { await audioService.playStopTransmittingSound(); } } });
494-498: Cleanup looks correct, minor formatting issueThe callback cleanup is appropriate. There's an extra blank line at 498 that could be removed.
// Cleanup CallKeep Mute Callback callKeepService.setMuteStateCallback(null); callKeepService.setEndCallCallback(null); - - + set({src/services/bluetooth-audio.service.ts (3)
135-141: Fix formatting: inconsistent indentationStatic analysis flagged extra spaces on lines 136-140. Apply consistent indentation.
♻️ Proposed fix
if (preferredDevice.id === 'system-audio') { - logger.info({ - message: 'Preferred device is System Audio, ensuring no specialized device is connected', - }); - // We are already in system audio mode by default if no device is connected - return; + logger.info({ + message: 'Preferred device is System Audio, ensuring no specialized device is connected', + }); + // We are already in system audio mode by default if no device is connected + return; }
591-615: Fix formatting ingetDeviceTypemethodStatic analysis flagged inconsistent indentation. Also consider extracting the specialized UUIDs to a constant for reusability.
♻️ Proposed fix for formatting
private getDeviceType(device: Device): 'specialized' | 'system' { const advertisingData = device.advertising; const serviceUUIDs = advertisingData?.serviceUUIDs || []; - + // Check for specialized PTT service UUIDs const isSpecialized = serviceUUIDs.some((uuid: string) => { - return [ - AINA_HEADSET_SERVICE, - B01INRICO_HEADSET_SERVICE, - HYS_HEADSET_SERVICE - ].some(specialized => this.areUuidsEqual(uuid, specialized)); + return [AINA_HEADSET_SERVICE, B01INRICO_HEADSET_SERVICE, HYS_HEADSET_SERVICE].some((specialized) => + this.areUuidsEqual(uuid, specialized) + ); }); if (isSpecialized) { return 'specialized'; } // Check by name for known specialized devices if UUID check fails const name = device.name?.toLowerCase() || ''; if (name.includes('aina') || name.includes('inrico') || name.includes('hys')) { - return 'specialized'; + return 'specialized'; } return 'system'; }
1019-1027: Remove or address verbose TODO commentsThe inline comments about storage persistence are implementation notes that should be addressed or removed before merging. If persistence is needed, implement it; otherwise, remove the commentary.
- // Save to storage (implied by previous code loading it from storage, but we need to save it too? - // usage of usePreferredBluetoothDevice hook elsewhere handles saving, - // but here we are in service. The UI calls logic that saves it eventually - // or we should do it here if we want persistence.) - // The service reads from storage using require('@/lib/storage'), so we should probably save it too if we want it to persist. - // However, the UI calls setPreferredDevice from the hook which likely saves it. - // We will let the UI handle the persistence call or add it here if needed. - // For now, updating the store is enough for the session. + // Note: UI handles persistence via usePreferredBluetoothDevice hook
| - Use React Navigation for handling navigation and deep linking with best practices. | ||
| - Create and use Jest to test to validate all generated components | ||
| - Generate tests for all components, services and logic generated. Ensure tests run without errors and fix any issues. | ||
| - The app is multi-lingual, so ensure all text is wrapped in `t()` from `react-i18next` for translations with the dictonary files stored in `src/translations`. |
There was a problem hiding this comment.
Typo: "dictonary" should be "dictionary"
-- The app is multi-lingual, so ensure all text is wrapped in `t()` from `react-i18next` for translations with the dictonary files stored in `src/translations`.
+- The app is multi-lingual, so ensure all text is wrapped in `t()` from `react-i18next` for translations with the dictionary files stored in `src/translations`.📝 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.
| - The app is multi-lingual, so ensure all text is wrapped in `t()` from `react-i18next` for translations with the dictonary files stored in `src/translations`. | |
| - The app is multi-lingual, so ensure all text is wrapped in `t()` from `react-i18next` for translations with the dictionary files stored in `src/translations`. |
🤖 Prompt for AI Agents
In @.agent/rules/rules.md at line 44, Fix the spelling mistake in the rule text:
replace the word "dictonary" with "dictionary" in the sentence that mentions
storing translation files in src/translations (the line containing "The app is
multi-lingual, so ensure all text is wrapped in `t()` from `react-i18next` for
translations with the dictonary files stored in `src/translations`.").
| <Text className={`font-medium ${preferredDevice?.id === 'system-audio' ? 'text-primary-700 dark:text-primary-300' : 'text-neutral-900 dark:text-neutral-100'}`}> | ||
| System Audio | ||
| </Text> | ||
| <Text className="text-xs text-neutral-500">AirPods, Car, Wired Headset</Text> |
There was a problem hiding this comment.
User-facing text missing i18n wrapping.
"System Audio" and "AirPods, Car, Wired Headset" are hardcoded strings. Per coding guidelines, all user-facing text should be wrapped in t().
🌐 Proposed fix for i18n
- <Text className={`font-medium ${preferredDevice?.id === 'system-audio' ? 'text-primary-700 dark:text-primary-300' : 'text-neutral-900 dark:text-neutral-100'}`}>
- System Audio
- </Text>
- <Text className="text-xs text-neutral-500">AirPods, Car, Wired Headset</Text>
+ <Text className={`font-medium ${preferredDevice?.id === 'system-audio' ? 'text-primary-700 dark:text-primary-300' : 'text-neutral-900 dark:text-neutral-100'}`}>
+ {t('bluetooth.system_audio')}
+ </Text>
+ <Text className="text-xs text-neutral-500">{t('bluetooth.system_audio_description')}</Text>Also addresses the static analysis hint about multiline formatting.
As per coding guidelines: "Ensure all text is wrapped in t() from react-i18next for translations with dictionary files stored in src/translations"
📝 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.
| <Text className={`font-medium ${preferredDevice?.id === 'system-audio' ? 'text-primary-700 dark:text-primary-300' : 'text-neutral-900 dark:text-neutral-100'}`}> | |
| System Audio | |
| </Text> | |
| <Text className="text-xs text-neutral-500">AirPods, Car, Wired Headset</Text> | |
| <Text className={`font-medium ${preferredDevice?.id === 'system-audio' ? 'text-primary-700 dark:text-primary-300' : 'text-neutral-900 dark:text-neutral-100'}`}> | |
| {t('bluetooth.system_audio')} | |
| </Text> | |
| <Text className="text-xs text-neutral-500">{t('bluetooth.system_audio_description')}</Text> |
🧰 Tools
🪛 GitHub Check: test
[warning] 276-276:
Replace ⏎····················System·Audio⏎·················· with System·Audio
🤖 Prompt for AI Agents
In `@src/components/settings/bluetooth-device-selection-bottom-sheet.tsx` around
lines 276 - 279, In the BluetoothDeviceSelectionBottomSheet component, replace
the hardcoded user-facing strings in the two <Text> elements ("System Audio" and
"AirPods, Car, Wired Headset") with calls to the i18n translator (t()) from
react-i18next (ensure t is imported/available in the component), e.g.
t('settings.systemAudio') and t('settings.systemAudioExamples'), and
update/create corresponding keys in the translations files; also ensure the JSX
formatting uses single-line expressions or proper multilines to satisfy the
static analyzer (keep each <Text> content as a single JS expression calling
t()).
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests