-
Notifications
You must be signed in to change notification settings - Fork 22
fix: request record permission on 14+ android versions #162
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdded Android 34+ microphone permission handling to the audio streamer hook. Introduced Changes
Sequence Diagram(s)sequenceDiagram
participant Code as Calling Code
participant Start as startForegroundServiceNotification()
participant Perm as ensureForegroundServiceMicPermission()
participant Platform as Platform/Permissions API
participant Alert as Alert Dialog
Code->>Start: initiate
Start->>Perm: await ensureForegroundServiceMicPermission()
Perm->>Platform: check platform & version
alt Android 34+
Perm->>Platform: request RECORD_AUDIO
Platform-->>Perm: permission result
alt Permission Granted
Perm->>Perm: log success
Perm-->>Start: return true
Start->>Platform: start foreground service notification
else Permission Denied
Perm->>Alert: show denial alert
Perm->>Perm: log denial
Perm-->>Start: return false
Start->>Start: abort (early return)
end
else Pre-Android 34
Perm->>Perm: log skipped
Perm-->>Start: return true
Start->>Platform: start foreground service notification
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (3)
app/app/hooks/useAudioStreamer.ts (3)
74-74: Improve Alert structure for better UX.The Alert displays a very long message as the first parameter without a proper title. Consider restructuring to use separate title and message parameters for better readability and UX.
Apply this diff to improve the Alert structure:
- Alert.alert('RECORD_AUDIO permission is required for microphone foreground service. Please grant microphone permission in app settings.'); + Alert.alert( + 'Microphone Permission Required', + 'Please grant microphone permission in app settings to use the audio recording feature.', + [{ text: 'OK' }] + );
70-72: Consider adding a rationale for better permission request UX.The permission request could benefit from providing a rationale explaining why the permission is needed. While the Alert handles denial, adding context upfront improves the user experience.
Consider adding a rationale to the permission request:
const audioResult = await PermissionsAndroid.request( - PermissionsAndroid.PERMISSIONS.RECORD_AUDIO + PermissionsAndroid.PERMISSIONS.RECORD_AUDIO, + { + title: 'Microphone Permission', + message: 'This app needs microphone access to record and stream audio.', + buttonPositive: 'Allow', + } );
59-85: Consider requesting permission earlier in the user flow.The permission is requested when starting the foreground service, which may interrupt the user experience at a critical moment. Consider requesting the RECORD_AUDIO permission earlier in the flow—such as during app onboarding, when the user first navigates to a recording screen, or when they tap a record button—before attempting to start the foreground service.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/app/hooks/useAudioStreamer.ts(2 hunks)
🔇 Additional comments (2)
app/app/hooks/useAudioStreamer.ts (2)
3-3: LGTM! Alert import correctly added.The Alert import is necessary for the permission denial notification and is properly added to the existing react-native imports.
60-62: Platform.Version check is correct.React Native's Platform.Version returns a number representing the Android API level on Android, so
Platform.Version < 34correctly identifies Android devices running versions below API level 34 (Android 14).
While testing the app on my Android simulator, the app crashed when I tried to record Audio.
It happened due to not having RECORD_AUDIO permission on Android 14+
Added necessary check for this permission on clicking record.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.