Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds logic to handle notification permission requests when users initially deny them. When permission is denied, the app now shows an alert dialog offering to take users to settings to enable notifications, with the option to exit the app. The implementation tracks when users navigate to settings and rechecks permissions when they return to the app.
Changes:
- Added alert dialog with "Exit App" and "Confirm" options when notification permissions are denied
- Implemented AppState listener to detect when users return from settings and recheck permissions
- Added module-level state tracking to determine when permission should be rechecked
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| services/notifications.ts | Added alert dialog for permission denial, module-level flag to track settings navigation, and exported function to check if permissions should be rechecked |
| app/_layout.tsx | Added AppState listener to detect app resume and trigger permission recheck when user returns from settings |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| onPress: () => { | ||
| wentToSettings = true; | ||
| Linking.openSettings(); | ||
| }, | ||
| }, | ||
| ]); |
There was a problem hiding this comment.
When the user clicks "확인" (Confirm) to go to settings, the flag wentToSettings is set to true before opening settings. However, if the user doesn't actually grant permissions in settings and returns to the app, the alert will not be shown again because the flag was already consumed by shouldRecheckPermission(). This means the user is stuck without notifications and no way to re-enable them except by reinstalling the app. Consider checking the actual permission status after returning from settings rather than just relying on the flag.
| import Constants from 'expo-constants'; | ||
| import { exitApp } from '@logicwind/react-native-exit-app'; | ||
|
|
||
| let wentToSettings = false; |
There was a problem hiding this comment.
The module-level state variable wentToSettings creates a potential race condition. If notificationPermissionAlert is triggered multiple times before the user acts, or if the user navigates to settings but the app state changes multiple times, the flag could be consumed by an unintended AppState change event. This can lead to unexpected behavior where permission checks are skipped or performed at incorrect times.
| } | ||
| if (finalStatus !== 'granted') { | ||
| handleRegistrationError('Permission not granted to get push token for push notification!'); | ||
| notificationPermissionAlert(); |
There was a problem hiding this comment.
The alert could potentially be shown multiple times if registerForPushNotificationsAsync is called concurrently (e.g., initial call and AppState change happening simultaneously). This creates a poor user experience with duplicate alert dialogs. Consider adding a guard to prevent showing the alert when one is already active.
| if (nextAppState === 'active' && shouldRecheckPermission()) { | ||
| registerForPushNotificationsAsync() | ||
| .then(handleToken) | ||
| .catch((error: any) => console.error(error)); |
There was a problem hiding this comment.
The function shouldRecheckPermission() is stateful and consumes the flag when called. However, it's called inline in the AppState event handler condition. If this condition is evaluated multiple times during the same render cycle or if multiple listeners exist, this could lead to the flag being consumed unexpectedly. Consider checking the flag and storing the result in a variable before using it in the condition, or refactoring to make the state management more explicit.
| if (nextAppState === 'active' && shouldRecheckPermission()) { | |
| registerForPushNotificationsAsync() | |
| .then(handleToken) | |
| .catch((error: any) => console.error(error)); | |
| if (nextAppState === 'active') { | |
| const shouldRecheck = shouldRecheckPermission(); | |
| if (shouldRecheck) { | |
| registerForPushNotificationsAsync() | |
| .then(handleToken) | |
| .catch((error: any) => console.error(error)); | |
| } |
| const subscription = AppState.addEventListener('change', (nextAppState) => { | ||
| if (nextAppState === 'active' && shouldRecheckPermission()) { | ||
| registerForPushNotificationsAsync() | ||
| .then(handleToken) | ||
| .catch((error: any) => console.error(error)); | ||
| } | ||
| }); |
There was a problem hiding this comment.
The AppState listener is set up immediately when the component mounts, but the initial permission check also runs immediately. This creates a potential race condition where both the initial registration and the AppState 'active' event could fire simultaneously, especially if the app regains focus during initialization. This could lead to duplicate permission requests or alert dialogs.
No description provided.