Develop #199
Develop #199
Conversation
📝 WalkthroughWalkthroughAdds Push-to-Talk (PTT) support for AirPods/Bluetooth earbuds via a new Expo config plugin that injects native iOS/Android modules, a MediaButtonService singleton for event handling and LiveKit microphone control, store extensions for PTT settings, and related docs, translations, tests, and minor image-permission error handling improvements. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (Earbud Button)
participant Native as iOS/Android Native Module
participant Service as MediaButtonService
participant LiveKit as LiveKit Room
participant Store as Bluetooth Audio Store
User->>Native: press / double-press media button
Native->>Service: emit onMediaButtonEvent(type, timestamp, source)
Service->>Service: detect single vs double-tap (timer)
alt PTT enabled & action maps to PTT
Service->>Store: get mediaButtonPTTSettings
Service->>LiveKit: request setMicrophoneEnabled(true/false)
LiveKit-->>Service: confirm microphone state
Service->>Service: play start/stop transmitting sound
Service->>Store: log PTT action
else Non-PTT or logging-only
Service->>Store: log media button event
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
.github/copilot-instructions.md (1)
51-51: Minor grammar improvement: hyphenate "Expo-managed".The static analysis tool flagged this, and using "Expo-managed" is grammatically correct when the compound modifier precedes the noun.
✏️ Suggested fix
-- This is an expo managed project that uses prebuild, do not make native code changes outside of expo prebuild capabilities. +- This is an Expo-managed project that uses prebuild, do not make native code changes outside of Expo prebuild capabilities.plugins/withMediaButtonModule.js (2)
485-491: Redundant branches: identical code in both if/else paths.Both branches of the
if (packagesMatch[1])condition produce the same replacement string. This appears to be copy-paste code where the "already has toMutableList()" case should differ from the "need to add toMutableList()" case, but currently they're identical, making the conditional check pointless.♻️ Suggested fix
Since both branches are identical, simplify to remove the unnecessary conditional:
if (packagesMatch) { - if (packagesMatch[1]) { - // Already has toMutableList() - mainApplication.contents = mainApplication.contents.replace(packagesPattern, `val packages = PackageList(this).packages.toMutableList()\n packages.add(MediaButtonPackage())`); - } else { - // Need to add toMutableList() - mainApplication.contents = mainApplication.contents.replace(packagesPattern, `val packages = PackageList(this).packages.toMutableList()\n packages.add(MediaButtonPackage())`); - } + mainApplication.contents = mainApplication.contents.replace( + packagesPattern, + `val packages = PackageList(this).packages.toMutableList()\n packages.add(MediaButtonPackage())` + ); console.log('[withMediaButtonModule] Registered MediaButtonPackage in MainApplication.kt'); }
299-305: Consider logging the exception during receiver unregistration.While catching the exception during cleanup is appropriate defensive coding, silently swallowing exceptions can make debugging harder. Consider logging at debug/warn level.
✏️ Suggested improvement
mediaButtonReceiver?.let { try { reactApplicationContext.unregisterReceiver(it) } catch (e: Exception) { - // Receiver may not be registered + // Receiver may not be registered - log for debugging + android.util.Log.d("MediaButtonModule", "Receiver unregistration skipped: \${e.message}") } }app.config.ts (1)
210-210: Align plugin filename with lowercase-hyphen conventionConsider renaming the plugin file to
with-media-button-module.jsand updating this reference to match the repo’s naming guideline. As per coding guidelines, ...♻️ Suggested update in this file (requires matching file rename)
- './plugins/withMediaButtonModule.js', + './plugins/with-media-button-module.js',src/services/media-button.service.ts (1)
1-40: Avoid duplicating PTT settings/types across modules
PTTModeandMediaButtonPTTSettingsduplicate definitions already present insrc/stores/app/bluetooth-audio-store.ts. Consider exporting shared types/defaults to prevent drift between modules.src/stores/app/bluetooth-audio-store.ts (2)
7-20: Consolidate PTT types to avoid drift.These definitions duplicate
src/services/media-button.service.tsLines 19-31, and the inline docs already diverge. Consider centralizingPTTModeandMediaButtonPTTSettingsin a shared types module and importing here to keep store/service aligned.
65-72: Avoid sharing a mutable default settings object.
DEFAULT_MEDIA_BUTTON_PTT_SETTINGSis exported and used directly as store state. If any consumer mutates it, the store state will mutate too. Consider cloning when initializing (and/or freezing the constant).♻️ Suggested change (clone on initialization)
- mediaButtonPTTSettings: DEFAULT_MEDIA_BUTTON_PTT_SETTINGS, + mediaButtonPTTSettings: { ...DEFAULT_MEDIA_BUTTON_PTT_SETTINGS },Also applies to: 156-156
| private handleSingleTap(type: MediaButtonEventType): void { | ||
| // Only handle play/pause type events for PTT | ||
| if (!this.settings.usePlayPauseForPTT) { | ||
| logger.debug({ | ||
| message: 'Play/Pause PTT disabled, ignoring', | ||
| context: { type }, | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| if (type === 'playPause' || type === 'play' || type === 'pause' || type === 'togglePlayPause') { | ||
| this.handlePTTAction(); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Handle double tap action | ||
| */ | ||
| private handleDoubleTap(): void { | ||
| if (this.settings.doubleTapAction === 'toggle_mute') { | ||
| this.handlePTTAction(); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Execute the PTT action (toggle or push-to-talk based on mode) | ||
| */ | ||
| private async handlePTTAction(): Promise<void> { | ||
| const liveKitStore = getLiveKitStore().getState(); | ||
|
|
||
| if (!liveKitStore.currentRoom) { | ||
| logger.debug({ | ||
| message: 'No active LiveKit room, cannot handle PTT action', | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| const currentMicEnabled = liveKitStore.currentRoom.localParticipant.isMicrophoneEnabled; | ||
| const newMicEnabled = !currentMicEnabled; | ||
|
|
||
| await liveKitStore.currentRoom.localParticipant.setMicrophoneEnabled(newMicEnabled); | ||
|
|
||
| // Create button event for store | ||
| const buttonEvent: AudioButtonEvent = { | ||
| type: 'press', | ||
| button: newMicEnabled ? 'ptt_start' : 'ptt_stop', | ||
| timestamp: Date.now(), | ||
| }; | ||
|
|
||
| useBluetoothAudioStore.getState().addButtonEvent(buttonEvent); | ||
| useBluetoothAudioStore.getState().setLastButtonAction({ | ||
| action: newMicEnabled ? 'unmute' : 'mute', | ||
| timestamp: Date.now(), | ||
| }); | ||
|
|
||
| // Play audio feedback | ||
| if (newMicEnabled) { | ||
| await audioService.playStartTransmittingSound(); | ||
| } else { | ||
| await audioService.playStopTransmittingSound(); | ||
| } | ||
|
|
||
| logger.info({ | ||
| message: 'PTT action executed via media button (AirPods/earbuds)', | ||
| context: { | ||
| micEnabled: newMicEnabled, | ||
| pttMode: this.settings.pttMode, | ||
| }, | ||
| }); | ||
| } catch (error) { | ||
| logger.error({ | ||
| message: 'Failed to execute PTT action via media button', | ||
| context: { error }, | ||
| }); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
pttMode is never honored (push-to-talk behaves like toggle)
pttMode is configured but not used—handlePTTAction always toggles mic state. This breaks push-to-talk semantics when users select that mode.
🛠️ Suggested fix: branch by pttMode in single-tap handling
private handleSingleTap(type: MediaButtonEventType): void {
// Only handle play/pause type events for PTT
if (!this.settings.usePlayPauseForPTT) {
logger.debug({
message: 'Play/Pause PTT disabled, ignoring',
context: { type },
});
return;
}
- if (type === 'playPause' || type === 'play' || type === 'pause' || type === 'togglePlayPause') {
- this.handlePTTAction();
- }
+ if (this.settings.pttMode === 'push_to_talk') {
+ if (type === 'play') {
+ void this.enableMicrophone();
+ } else if (type === 'pause') {
+ void this.disableMicrophone();
+ } else if (type === 'playPause' || type === 'togglePlayPause') {
+ void this.handlePTTAction(); // fallback when press/release isn’t available
+ }
+ return;
+ }
+
+ if (type === 'playPause' || type === 'play' || type === 'pause' || type === 'togglePlayPause') {
+ void this.handlePTTAction();
+ }
}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/stores/app/bluetooth-audio-store.ts`:
- Around line 4-9: The multi-line import of createDefaultPTTSettings,
DEFAULT_MEDIA_BUTTON_PTT_SETTINGS, MediaButtonPTTSettings, and PTTMode violates
the project's import formatting rule; update the import statement in
bluetooth-audio-store.ts to a formatter-compliant form (collapse into a single
line or run the project's formatter) so the imports for
createDefaultPTTSettings, DEFAULT_MEDIA_BUTTON_PTT_SETTINGS, type
MediaButtonPTTSettings, and type PTTMode are formatted per lint rules.
♻️ Duplicate comments (1)
src/services/media-button.service.ts (1)
260-272:pttModestill not honored (push‑to‑talk behaves like toggle).
Single-tap handling ignorespttModeand always toggles, so press‑and‑hold behavior never happens. This is the same issue previously raised.🛠️ Suggested fix (branch by pttMode)
private handleSingleTap(type: MediaButtonEventType): void { // Only handle play/pause type events for PTT if (!this.settings.usePlayPauseForPTT) { logger.debug({ Same as before, review concisely with a diff or approach. </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (1)</summary><blockquote> <details> <summary>plugins/withMediaButtonModule.js (1)</summary><blockquote> `1-4`: **Filename doesn’t match the lowercase-hyphenated convention.** Consider renaming `plugins/withMediaButtonModule.js` → `plugins/with-media-button-module.js` and updating references to align with the project’s naming guidelines. </blockquote></details> </blockquote></details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| import { | ||
| createDefaultPTTSettings, | ||
| DEFAULT_MEDIA_BUTTON_PTT_SETTINGS, | ||
| type MediaButtonPTTSettings, | ||
| type PTTMode, | ||
| } from '@/types/ptt'; |
There was a problem hiding this comment.
Lint warning: import formatting.
Static analysis flags the multi-line import here; please run the formatter or collapse the import into a single line per the lint rule.
🧹 Example formatter-compliant import
-import {
- createDefaultPTTSettings,
- DEFAULT_MEDIA_BUTTON_PTT_SETTINGS,
- type MediaButtonPTTSettings,
- type PTTMode,
-} from '@/types/ptt';
+import { createDefaultPTTSettings, DEFAULT_MEDIA_BUTTON_PTT_SETTINGS, type MediaButtonPTTSettings, type PTTMode } from '@/types/ptt';📝 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 { | |
| createDefaultPTTSettings, | |
| DEFAULT_MEDIA_BUTTON_PTT_SETTINGS, | |
| type MediaButtonPTTSettings, | |
| type PTTMode, | |
| } from '@/types/ptt'; | |
| import { createDefaultPTTSettings, DEFAULT_MEDIA_BUTTON_PTT_SETTINGS, type MediaButtonPTTSettings, type PTTMode } from '@/types/ptt'; |
🧰 Tools
🪛 GitHub Check: test
[warning] 4-4:
Replace ⏎··createDefaultPTTSettings,⏎··DEFAULT_MEDIA_BUTTON_PTT_SETTINGS,⏎··type·MediaButtonPTTSettings,⏎··type·PTTMode,⏎ with ·createDefaultPTTSettings,·DEFAULT_MEDIA_BUTTON_PTT_SETTINGS,·type·MediaButtonPTTSettings,·type·PTTMode·
🤖 Prompt for AI Agents
In `@src/stores/app/bluetooth-audio-store.ts` around lines 4 - 9, The multi-line
import of createDefaultPTTSettings, DEFAULT_MEDIA_BUTTON_PTT_SETTINGS,
MediaButtonPTTSettings, and PTTMode violates the project's import formatting
rule; update the import statement in bluetooth-audio-store.ts to a
formatter-compliant form (collapse into a single line or run the project's
formatter) so the imports for createDefaultPTTSettings,
DEFAULT_MEDIA_BUTTON_PTT_SETTINGS, type MediaButtonPTTSettings, and type PTTMode
are formatted per lint rules.
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.