-
Notifications
You must be signed in to change notification settings - Fork 5
RR-T40 Moving phone permission check to voice call instead of startup. #105
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
Conversation
📝 WalkthroughWalkthroughAdds behavioral guidance rules for AI problem-solving; refactors Android CallKeep permission handling from setup-time to on-demand via new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
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: 0
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.android.ts (1)
60-63:⚠️ Potential issue | 🟠 MajorRemove
READ_PHONE_NUMBERSfromadditionalPermissionsinsetup()to avoid conflicting with the on-demand request at call time.The
additionalPermissionsarray at lines 60–63 includesREAD_PHONE_NUMBERS(on API 30+), which will trigger an Android runtime permission dialog duringRNCallKeep.setup(). This conflicts with the stated goal in your comment (lines 78–79) to request phone permissions on-demand when connecting a voice call.Since
requestPhonePermissions()already handles theREAD_PHONE_NUMBERSrequest at call time, remove it fromadditionalPermissionson API 30+:Suggested change
additionalPermissions: [ PermissionsAndroid.PERMISSIONS.READ_PHONE_STATE, - ...(Platform.Version >= 30 ? [PermissionsAndroid.PERMISSIONS.READ_PHONE_NUMBERS] : []), ],
READ_PHONE_STATEcan remain as it may be needed during setup, butREAD_PHONE_NUMBERSshould only be requested just before the call.
🧹 Nitpick comments (2)
src/services/callkeep.service.android.ts (2)
100-151: Core permission logic looks correct; fix formatting per linter.The
requestPhonePermissions()method is well-structured: early returns for non-Android / pre-API-30, check-before-request pattern, proper error handling, and non-blocking return values.Static analysis flagged formatting issues (extra indentation) in the
PermissionsAndroid.checkandPermissionsAndroid.requestcall sites. Please fix to match project formatting rules.🧹 Proposed formatting fix
const hasPermission = await PermissionsAndroid.check( - PermissionsAndroid.PERMISSIONS.READ_PHONE_NUMBERS - ); + PermissionsAndroid.PERMISSIONS.READ_PHONE_NUMBERS, + ); // ... const result = await PermissionsAndroid.request( - PermissionsAndroid.PERMISSIONS.READ_PHONE_NUMBERS, - { - title: 'Phone Permission Required', - message: 'This app needs phone access to manage voice calls with your headset', - buttonPositive: 'Grant', - buttonNegative: 'Deny', - } + PermissionsAndroid.PERMISSIONS.READ_PHONE_NUMBERS, + { + title: 'Phone Permission Required', + message: 'This app needs phone access to manage voice calls with your headset', + buttonPositive: 'Grant', + buttonNegative: 'Deny', + }, );
127-135: User-facing permission rationale strings are not internationalized.The
title,message,buttonPositive, andbuttonNegativestrings are hardcoded in English. Since the app is multi-lingual, these should be wrapped int()fromreact-i18nextfor localization. As per coding guidelines: "The app is multi-lingual, so ensure all text is wrapped int()fromreact-i18nextfor translations."
|
Approve |
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.
This PR is approved.
Summary by CodeRabbit