[PC-1627] 알람 권한 티라미수 이하 버전 로직 수정 #203
Conversation
📝 WalkthroughWalkthroughLifecycle-aware permission state was added and notification handling unified: notification enablement now checks NotificationManagerCompat.areNotificationsEnabled(); on Android TIRAMISU+ the POST_NOTIFICATIONS runtime permission is requested, while on older APIs the flow opens the app's notification settings via a shared navigateToAppSettings helper. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as App UI (Toggle)
participant PermAPI as Runtime Permission (POST_NOTIFICATIONS)
participant SysMgr as System Notification Manager / Settings
User->>UI: Toggle Push Notifications
UI->>SysMgr: check areNotificationsEnabled()
alt Android >= TIRAMISU
UI->>PermAPI: check POST_NOTIFICATIONS state
PermAPI-->>UI: Granted / Denied
alt Granted
UI-->>User: Enable toggle (persist)
else Denied
UI->>PermAPI: requestPermission()
PermAPI-->>User: system permission dialog
PermAPI-->>UI: result (Granted/Denied)
alt Granted
UI-->>User: Enable toggle (persist)
else Denied
UI->>UI: optionally navigateToAppSettings() (if non-rationale)
UI-->>User: Prompt to open app settings
end
end
else Android < TIRAMISU
SysMgr-->>UI: areNotificationsEnabled() result
alt Enabled
UI-->>User: Enable toggle (persist)
else Disabled
UI->>SysMgr: open ACTION_APPLICATION_DETAILS_SETTINGS (via navigateToAppSettings)
SysMgr-->>User: show app notification settings
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ 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: 0
🧹 Nitpick comments (2)
feature/auth/src/main/java/com/puzzle/auth/graph/signup/page/AccessRightsPage.kt (2)
101-107: Consider optimizing lifecycle state dependency.The current implementation recomputes
isNotificationGrantedon every lifecycle state change. WhileareNotificationsEnabled()is lightweight, you could optimize by filtering for specific states or usingderivedStateOfto reduce unnecessary recompositions.♻️ Optional optimization
- val isNotificationGranted = remember(lifecycleState, notificationPermission?.status) { + val isNotificationGranted = remember( + key1 = if (lifecycleState.isAtLeast(Lifecycle.State.RESUMED)) lifecycleState else null, + key2 = notificationPermission?.status + ) { if (SDK_INT >= TIRAMISU) { notificationPermission?.status == PermissionStatus.Granted } else { NotificationManagerCompat.from(context).areNotificationsEnabled() } }This limits recomputation to when the screen becomes visible (RESUMED state), which is when the user might return from settings.
255-266: Extract duplicatedhandlePermissionto shared utility.The
handlePermissionfunction is duplicated in bothAccessRightsPage.ktandSettingScreen.ktwith identical logic. Consider extracting it to a common utility module to improve maintainability.♻️ Suggested refactor
Create a shared utility file (e.g.,
core/common/src/main/java/com/puzzle/common/permission/PermissionUtils.kt):package com.puzzle.common.permission import android.content.Context import android.content.Intent import android.net.Uri import android.provider.Settings import com.google.accompanist.permissions.ExperimentalPermissionsApi import com.google.accompanist.permissions.PermissionState import com.google.accompanist.permissions.PermissionStatus import com.google.accompanist.permissions.shouldShowRationale @OptIn(ExperimentalPermissionsApi::class) fun handlePermission(context: Context, permission: PermissionState?) { permission?.let { if (it.status == PermissionStatus.Granted || !it.status.shouldShowRationale) { val intent = Intent(Settings.ACTION_APPLICATION_DETAILS_SETTINGS).apply { data = Uri.fromParts("package", context.packageName, null) } context.startActivity(intent) } else { it.launchPermissionRequest() } } }Then import and use it in both files.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
feature/auth/src/main/java/com/puzzle/auth/graph/signup/page/AccessRightsPage.kt
🧰 Additional context used
🧬 Code graph analysis (1)
feature/auth/src/main/java/com/puzzle/auth/graph/signup/page/AccessRightsPage.kt (2)
feature/auth/src/main/java/com/puzzle/auth/graph/signup/SignUpViewModel.kt (1)
onBackClick(112-121)feature/setting/src/main/java/com/puzzle/setting/graph/main/SettingScreen.kt (1)
handlePermission(401-413)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (5)
feature/auth/src/main/java/com/puzzle/auth/graph/signup/page/AccessRightsPage.kt (5)
32-33: LGTM! Imports support lifecycle-aware permission handling.The added imports are necessary for the new implementation: lifecycle state tracking, notification status checking on older APIs, and state management.
Also applies to: 44-46
67-70: LGTM! Lifecycle tracking enables state refresh on resume.Collecting lifecycle state allows the notification permission status to be rechecked when the user returns from system settings, which is essential for pre-Tiramisu devices.
92-94: LGTM! Correct API-level handling for notification permission.The conditional retrieval correctly reflects that
POST_NOTIFICATIONSruntime permission only exists on API 33+.
109-114: LGTM! Conditional permission request prevents unnecessary prompts.The change to
LaunchedEffect(Unit)with ahasDeniedcheck correctly implements the requirement to show already-granted permissions without immediately launching a permission request. This improves UX by respecting previously granted permissions.
155-165: LGTM! Correct API-level branching for notification settings.The implementation correctly handles both API levels:
- API 33+: Uses runtime permission flow via
handlePermission- Below API 33: Opens notification settings directly, since no runtime permission exists
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
@feature/setting/src/main/java/com/puzzle/setting/graph/main/SettingScreen.kt:
- Around line 230-237: The pre-TIRAMISU notification permission check in
NotificationBody is computed on every recomposition and isn’t lifecycle-aware,
so the UI won’t refresh when the user toggles notifications in system settings;
change this by hoisting isPermissionGranted into a remembered MutableState
(e.g., remember { mutableStateOf(...) }) and update that state from a
lifecycle-aware coroutine (use lifecycleOwner.repeatOnStarted or a
LaunchedEffect tied to lifecycleOwner) to call
NotificationManagerCompat.from(context).areNotificationsEnabled() on resume;
reference NotificationBody for where to create and read the state, and mirror
the lifecycle pattern used in SettingRoute and AccessRightsPage to trigger a
state refresh when the screen resumes.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
feature/setting/src/main/java/com/puzzle/setting/graph/main/SettingScreen.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (1)
feature/setting/src/main/java/com/puzzle/setting/graph/main/SettingScreen.kt (1)
43-43: LGTM! Necessary additions for pre-TIRAMISU support.The
NotificationManagerCompatimport andLocalContextretrieval are correctly added to support the new notification permission logic for Android versions below TIRAMISU.Also applies to: 224-224
feature/setting/src/main/java/com/puzzle/setting/graph/main/SettingScreen.kt
Outdated
Show resolved
Hide resolved
| if (SDK_INT >= TIRAMISU) { | ||
| handlePermission(context, notificationPermission) | ||
| } else { | ||
| val intent = Intent(Settings.ACTION_APP_NOTIFICATION_SETTINGS).apply { | ||
| putExtra(Settings.EXTRA_APP_PACKAGE, context.packageName) | ||
| } | ||
| context.startActivity(intent) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Settings.ACTION_APP_NOTIFICATION_SETTINGS EXTRA_APP_PACKAGE Android API 21 32 documentation
💡 Result:
Summary — official docs and practical compatibility:
-
Official (Android SDK): Settings.ACTION_APP_NOTIFICATION_SETTINGS and Settings.EXTRA_APP_PACKAGE were added in API level 26 (Android O). Use Intent action Settings.ACTION_APP_NOTIFICATION_SETTINGS with extra Settings.EXTRA_APP_PACKAGE (android.provider.extra.APP_PACKAGE). [1]
-
Pre-O compatibility (API 21–25): many apps use the settings action string "android.settings.APP_NOTIFICATION_SETTINGS" and the undocumented extras "app_package" (String) and "app_uid" (int) to open an app's notification settings on Lollipop—Marshmallow—Nougat devices. This approach is not part of the public SDK and may not work on all devices. Example usages and community guidance are shown on Stack Overflow. [2]
References:
[1] Android Developers — Settings (API reference) (shows ACTION_APP_NOTIFICATION_SETTINGS and EXTRA_APP_PACKAGE added in API 26).
[2] Stack Overflow — “Any way to link to the Android notification settings for my app?” (compatibility examples for API 21–25 using "app_package" / "app_uid").
Fix Intent approach for API 21-25 compatibility.
The code uses Settings.ACTION_APP_NOTIFICATION_SETTINGS and Settings.EXTRA_APP_PACKAGE, which were added in API level 26. These constants are unavailable on API 21-25, causing the pre-TIRAMISU branch (which includes API 21-25) to crash when executed on those devices.
For API 21-25 compatibility, use the undocumented Intent action "android.settings.APP_NOTIFICATION_SETTINGS" with extras "app_package" (String) and "app_uid" (int) instead. Add a version check:
- API 26+: Use
Settings.ACTION_APP_NOTIFICATION_SETTINGSwithSettings.EXTRA_APP_PACKAGE - API 21-25: Use the undocumented action string with
"app_package"and"app_uid"extras
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Piece는 현재 minSDK가 28이어서 API 21~25 호환성 이슈는 괜찮을 거 같습니다.
tgyuuAn
left a comment
tgyuuAn
left a comment
There was a problem hiding this comment.
해당 PR은 어떤 맥락에서 등장한 PR일까요?
PR에 Context가 빈약한 것 같아요.
코드가 작성된 이유도 작성되었으면 좋겠어요!
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@feature/setting/src/main/java/com/puzzle/setting/graph/main/SettingScreen.kt:
- Around line 235-246: The permission check in SettingScreen.kt for
isPermissionGranted is inconsistent with AccessRightsPage.kt: update the
remember block used to compute isPermissionGranted so that on SDK_INT >=
TIRAMISU you return true only if notifications are enabled OR the runtime
notificationPermission?.status == PermissionStatus.Granted (i.e., use the same
areNotificationsEnabled() || notificationPermission?.status ==
PermissionStatus.Granted logic used in AccessRightsPage.kt), referencing
NotificationManagerCompat.from(context).areNotificationsEnabled() and the
notificationPermission variable to ensure both system setting and runtime
permission are considered.
🧹 Nitpick comments (1)
feature/setting/src/main/java/com/puzzle/setting/graph/main/SettingScreen.kt (1)
433-444: Extract duplicated function to shared utility.This
handlePermissionfunction is duplicated inAccessRightsPage.kt(lines 257-268). Consider extracting it to a common utility module or package to maintain DRY principles and ensure consistent behavior across the app.♻️ Suggestion: Extract to common module
Create a shared utility file like
feature-common/src/main/java/com/puzzle/common/permission/PermissionUtils.kt:@OptIn(ExperimentalPermissionsApi::class) fun handlePermission(context: Context, permission: PermissionState?) { permission?.let { if (it.status == PermissionStatus.Granted || !it.status.shouldShowRationale) { val intent = Intent(Settings.ACTION_APPLICATION_DETAILS_SETTINGS).apply { data = Uri.fromParts("package", context.packageName, null) } context.startActivity(intent) } else { it.launchPermissionRequest() } } }Then import and use it in both files.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
feature/auth/src/main/java/com/puzzle/auth/graph/signup/page/AccessRightsPage.ktfeature/setting/src/main/java/com/puzzle/setting/graph/main/SettingScreen.kt
🧰 Additional context used
🧬 Code graph analysis (2)
feature/setting/src/main/java/com/puzzle/setting/graph/main/SettingScreen.kt (1)
feature/auth/src/main/java/com/puzzle/auth/graph/signup/page/AccessRightsPage.kt (1)
handlePermission(256-268)
feature/auth/src/main/java/com/puzzle/auth/graph/signup/page/AccessRightsPage.kt (1)
feature/setting/src/main/java/com/puzzle/setting/graph/main/SettingScreen.kt (1)
handlePermission(432-444)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (6)
feature/setting/src/main/java/com/puzzle/setting/graph/main/SettingScreen.kt (2)
248-252: Review auto-enable behavior for UX implications.This effect automatically enables push notifications when permission is granted, even if the user hasn't explicitly toggled it on. While this may be intentional, consider whether users who grant permission from system settings might want to manually control the toggle.
For example: A user grants notification permission but may still want to decide later whether to enable push notifications in the app.
Is this auto-enable behavior the intended UX? If so, consider adding a comment explaining the rationale.
280-287: LGTM! Correct pre-TIRAMISU behavior.The conditional logic correctly handles both API levels:
- TIRAMISU+: Uses runtime permission flow
- Pre-TIRAMISU: Opens system notification settings directly
This aligns with the PR objectives to avoid showing permission popup on older Android versions.
feature/auth/src/main/java/com/puzzle/auth/graph/signup/page/AccessRightsPage.kt (4)
68-69: LGTM! Lifecycle-aware state handling.The lifecycle state collection enables the UI to update when the user returns from the system settings screen, which is essential for reflecting permission changes.
101-109: LGTM! Correct combined permission check.The logic correctly combines both checks for TIRAMISU+:
- Runtime permission status (
POST_NOTIFICATIONS)- System notification settings (
areNotificationsEnabled())This is the proper approach since on Android 13+, both can be controlled independently.
Note: Ensure
SettingScreen.ktuses this same logic (see comment on that file).
111-116: LGTM! Improved permission request logic.Changing to
LaunchedEffect(Unit)ensures permissions are only requested once during initial composition, and thehasDeniedcheck prevents unnecessary requests when all permissions are already granted.
157-167: LGTM! Consistent pre-TIRAMISU handling.The notification row correctly:
- Uses the combined
isNotificationGrantedstate for the toggle- Routes to appropriate handlers based on API level
- Opens system notification settings directly on pre-TIRAMISU devices
This aligns with the PR objectives and matches the approach in
SettingScreen.kt.
| val isPermissionGranted = remember( | ||
| lifecycleState, | ||
| notificationPermission?.status | ||
| ) { | ||
| if (SDK_INT >= TIRAMISU) { | ||
| notificationPermission?.status == PermissionStatus.Granted | ||
| } else { | ||
| NotificationManagerCompat | ||
| .from(context) | ||
| .areNotificationsEnabled() | ||
| } | ||
| } |
There was a problem hiding this comment.
Inconsistent permission check logic for TIRAMISU+ between files.
For Android 13+ (TIRAMISU), this code only checks the runtime permission status, but in AccessRightsPage.kt (lines 104-108), the logic uses:
areNotificationsEnabled || notificationPermission?.status == PermissionStatus.GrantedThis inconsistency could cause issues because:
- On TIRAMISU+, users can grant the runtime permission but still disable notifications in system settings
- The two screens would show different permission states for the same notification status
🔧 Proposed fix to align with AccessRightsPage.kt logic
val isPermissionGranted = remember(
lifecycleState,
notificationPermission?.status
) {
+ val areNotificationsEnabled = NotificationManagerCompat
+ .from(context)
+ .areNotificationsEnabled()
+
if (SDK_INT >= TIRAMISU) {
- notificationPermission?.status == PermissionStatus.Granted
+ areNotificationsEnabled || notificationPermission?.status == PermissionStatus.Granted
} else {
- NotificationManagerCompat
- .from(context)
- .areNotificationsEnabled()
+ areNotificationsEnabled
}
}🤖 Prompt for AI Agents
In @feature/setting/src/main/java/com/puzzle/setting/graph/main/SettingScreen.kt
around lines 235 - 246, The permission check in SettingScreen.kt for
isPermissionGranted is inconsistent with AccessRightsPage.kt: update the
remember block used to compute isPermissionGranted so that on SDK_INT >=
TIRAMISU you return true only if notifications are enabled OR the runtime
notificationPermission?.status == PermissionStatus.Granted (i.e., use the same
areNotificationsEnabled() || notificationPermission?.status ==
PermissionStatus.Granted logic used in AccessRightsPage.kt), referencing
NotificationManagerCompat.from(context).areNotificationsEnabled() and the
notificationPermission variable to ensure both system setting and runtime
permission are considered.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| val isPermissionGranted = remember( | ||
| lifecycleState, | ||
| notificationPermission?.status | ||
| ) { | ||
| if (SDK_INT >= TIRAMISU) { | ||
| notificationPermission?.status == PermissionStatus.Granted | ||
| } else { | ||
| NotificationManagerCompat | ||
| .from(context) | ||
| .areNotificationsEnabled() | ||
| } | ||
| } |
| if (SDK_INT >= TIRAMISU) { | ||
| handlePermission(context, notificationPermission) | ||
| } else { | ||
| val intent = Intent(Settings.ACTION_APP_NOTIFICATION_SETTINGS).apply { | ||
| putExtra(Settings.EXTRA_APP_PACKAGE, context.packageName) | ||
| } | ||
| context.startActivity(intent) | ||
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
feature/auth/src/main/java/com/puzzle/auth/graph/signup/page/AccessRightsPage.kt (1)
253-265: Consider extractinghandlePermissionto a shared utility.This function is duplicated in
SettingScreen.kt(lines 426-438). Consider moving it to a common module (e.g.,com.puzzle.common.permission) to avoid duplication and ensure consistent behavior across both files.♻️ Suggested refactor
Create a shared utility in the common module:
// common/src/main/java/com/puzzle/common/permission/PermissionUtils.kt package com.puzzle.common.permission import android.content.Context import android.content.Intent import android.net.Uri import android.provider.Settings import com.google.accompanist.permissions.ExperimentalPermissionsApi import com.google.accompanist.permissions.PermissionState import com.google.accompanist.permissions.PermissionStatus import com.google.accompanist.permissions.shouldShowRationale @OptIn(ExperimentalPermissionsApi::class) fun handlePermission(context: Context, permission: PermissionState?) { permission?.let { if (it.status == PermissionStatus.Granted || !it.status.shouldShowRationale) { val intent = Intent(Settings.ACTION_APPLICATION_DETAILS_SETTINGS).apply { data = Uri.fromParts("package", context.packageName, null) } context.startActivity(intent) } else { it.launchPermissionRequest() } } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
feature/auth/src/main/java/com/puzzle/auth/graph/signup/page/AccessRightsPage.ktfeature/setting/src/main/java/com/puzzle/setting/graph/main/SettingScreen.kt
🧰 Additional context used
🧬 Code graph analysis (2)
feature/auth/src/main/java/com/puzzle/auth/graph/signup/page/AccessRightsPage.kt (2)
feature/auth/src/main/java/com/puzzle/auth/graph/signup/SignUpViewModel.kt (1)
onBackClick(112-121)feature/setting/src/main/java/com/puzzle/setting/graph/main/SettingScreen.kt (1)
handlePermission(426-438)
feature/setting/src/main/java/com/puzzle/setting/graph/main/SettingScreen.kt (1)
feature/auth/src/main/java/com/puzzle/auth/graph/signup/page/AccessRightsPage.kt (1)
handlePermission(253-265)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (5)
feature/setting/src/main/java/com/puzzle/setting/graph/main/SettingScreen.kt (2)
223-246: LGTM! Lifecycle-aware permission state handling.The use of
lifecycleStateas arememberkey correctly triggers recomputation when the user returns from the settings screen, ensuring the UI reflects the current notification permission state.One edge case to verify: on first composition, if permission is already granted but
isPushNotificationEnabledisfalse(e.g., server-side state mismatch), theLaunchedEffectwill immediately callonPushNotificationCheckedChange(). Ensure this auto-sync behavior is intended.
274-281: LGTM! Correct SDK-aware navigation for pre-Tiramisu devices.Using
Settings.ACTION_APP_NOTIFICATION_SETTINGSwithEXTRA_APP_PACKAGEis the correct approach for Android versions below Tiramisu where runtimePOST_NOTIFICATIONSpermission doesn't exist.feature/auth/src/main/java/com/puzzle/auth/graph/signup/page/AccessRightsPage.kt (3)
67-106: LGTM! Consistent lifecycle-aware permission handling.The lifecycle-aware pattern matches the implementation in
SettingScreen.kt, ensuring consistent behavior across both flows when users return from system settings.
108-113: Good improvement: Conditional permission request.Changing to
LaunchedEffect(Unit)with thehasDeniedcheck prevents unnecessary permission prompts when all permissions are already granted. This is a better UX than unconditionally launching the request.
154-164: LGTM! SDK-aware notification settings navigation.Consistent with
SettingScreen.kt- uses runtime permission handling for TIRAMISU+ and directly opens system notification settings for older versions.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@feature/setting/src/main/java/com/puzzle/setting/graph/main/SettingScreen.kt:
- Around line 242-246: The current LaunchedEffect triggers on any
isPermissionGranted truthy state and calls onPushNotificationCheckedChange(),
which can unintentionally re-enable notifications on first composition or when
permission was already granted; change this to detect a transition from
false->true by adding a remembered state like previousPermissionGranted
(rememberSaveable { isPermissionGranted }) or previousPermissionGranted =
remember { mutableStateOf(isPermissionGranted) }, run
onPushNotificationCheckedChange() only when previousPermissionGranted == false
&& isPermissionGranted == true && !isPushNotificationEnabled, and then update
previousPermissionGranted = isPermissionGranted so only fresh grants (not
pre-existing permissions) trigger the callback.
🧹 Nitpick comments (2)
feature/auth/src/main/java/com/puzzle/auth/graph/signup/page/AccessRightsPage.kt (2)
108-113: Consider adding a guard to prevent repeated permission requests on configuration changes.
LaunchedEffect(Unit)runs once per composition lifetime. However, if there are configuration changes (e.g., rotation), this could trigger again. If this is intentional behavior, consider adding a comment to clarify; otherwise, you may want to track whether the request has already been made.
251-259: Consider extracting shared permission utilities to a common module.
handlePermissionandnavigateToAppSettingsare duplicated between this file andSettingScreen.kt. Additionally,handlePermissionhas inconsistent visibility (internalhere vsprivatein SettingScreen.kt).Consider extracting these to a shared utility in the common module to reduce duplication and ensure consistent behavior.
💡 Suggested approach
Create a shared utility file (e.g.,
PermissionUtils.kt) in the common module:// common/src/main/java/com/puzzle/common/util/PermissionUtils.kt @OptIn(ExperimentalPermissionsApi::class) fun handlePermission(context: Context, permission: PermissionState?) { permission?.let { if (it.status == PermissionStatus.Granted || !it.status.shouldShowRationale) { navigateToAppSettings(context) } else { it.launchPermissionRequest() } } } fun navigateToAppSettings(context: Context) { context.startActivity( Intent(Settings.ACTION_APPLICATION_DETAILS_SETTINGS).apply { data = Uri.fromParts("package", context.packageName, null) } ) }Also applies to: 261-267
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
feature/auth/src/main/java/com/puzzle/auth/graph/signup/page/AccessRightsPage.ktfeature/setting/src/main/java/com/puzzle/setting/graph/main/SettingScreen.kt
🧰 Additional context used
🧬 Code graph analysis (2)
feature/setting/src/main/java/com/puzzle/setting/graph/main/SettingScreen.kt (1)
feature/auth/src/main/java/com/puzzle/auth/graph/signup/page/AccessRightsPage.kt (2)
handlePermission(250-259)navigateToAppSettings(261-267)
feature/auth/src/main/java/com/puzzle/auth/graph/signup/page/AccessRightsPage.kt (2)
feature/auth/src/main/java/com/puzzle/auth/graph/signup/SignUpViewModel.kt (1)
onBackClick(112-121)feature/setting/src/main/java/com/puzzle/setting/graph/main/SettingScreen.kt (2)
handlePermission(423-432)navigateToAppSettings(434-440)
🔇 Additional comments (8)
feature/auth/src/main/java/com/puzzle/auth/graph/signup/page/AccessRightsPage.kt (4)
67-69: LGTM! Lifecycle-aware state collection.Good approach to observe lifecycle state changes so the UI updates when the user returns from the system settings screen.
92-94: LGTM! Conditional permission retrieval for TIRAMISU+.Correctly returns
nullfor pre-TIRAMISU devices sincePOST_NOTIFICATIONSpermission doesn't exist on those versions.
101-106: LGTM! Lifecycle-aware notification grant check.Using
rememberwithlifecycleStateas a key ensures the notification status is re-evaluated when the user returns from settings.
154-161: LGTM! API-aware notification permission handling.Correctly routes TIRAMISU+ to the runtime permission flow and pre-TIRAMISU directly to app settings.
feature/setting/src/main/java/com/puzzle/setting/graph/main/SettingScreen.kt (4)
223-230: LGTM! Lifecycle-aware state for settings screen.Consistent with the pattern used in
AccessRightsPage.ktto refresh permission status when returning from system settings.
235-240: LGTM! Lifecycle-aware permission check.Using
NotificationManagerCompat.areNotificationsEnabled()with lifecycle state as a key correctly handles both TIRAMISU+ and pre-TIRAMISU devices.
274-278: LGTM! API-aware permission handling for notification toggle.Correctly routes pre-TIRAMISU devices directly to app settings since runtime permission isn't available.
423-440: LGTM! Private visibility and consistent implementation.The helper functions are appropriately scoped as private. Note: As mentioned in the other file's review, consider extracting these to a shared utility to eliminate duplication.
| LaunchedEffect(isPermissionGranted) { | ||
| if (isPermissionGranted && !isPushNotificationEnabled) { | ||
| onPushNotificationCheckedChange() | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential unintended auto-enable of push notifications.
This LaunchedEffect automatically calls onPushNotificationCheckedChange() when permission becomes granted and push notifications are disabled. This could cause issues:
- If the user intentionally disabled push notifications in the app but has system permission enabled, this would re-enable them on every screen visit.
- On first composition where permission is already granted, this will always trigger.
Consider tracking whether the user explicitly granted permission in this session vs. permission already being granted.
💡 Suggested approach
Track whether this is a fresh permission grant vs. pre-existing:
+ var wasPermissionGrantedOnEntry by remember { mutableStateOf(isPermissionGranted) }
+
LaunchedEffect(isPermissionGranted) {
- if (isPermissionGranted && !isPushNotificationEnabled) {
+ if (isPermissionGranted && !wasPermissionGrantedOnEntry && !isPushNotificationEnabled) {
onPushNotificationCheckedChange()
}
}🤖 Prompt for AI Agents
In @feature/setting/src/main/java/com/puzzle/setting/graph/main/SettingScreen.kt
around lines 242 - 246, The current LaunchedEffect triggers on any
isPermissionGranted truthy state and calls onPushNotificationCheckedChange(),
which can unintentionally re-enable notifications on first composition or when
permission was already granted; change this to detect a transition from
false->true by adding a remembered state like previousPermissionGranted
(rememberSaveable { isPermissionGranted }) or previousPermissionGranted =
remember { mutableStateOf(isPermissionGranted) }, run
onPushNotificationCheckedChange() only when previousPermissionGranted == false
&& isPermissionGranted == true && !isPushNotificationEnabled, and then update
previousPermissionGranted = isPermissionGranted so only fresh grants (not
pre-existing permissions) trigger the callback.
There was a problem hiding this comment.
알람 정책:
시스템 알림 권한을 다시 활성화한 경우, 사용자가 알림 수신 의사를 회복한 것으로 판단하여 앱 및 서버 푸시 설정을 자동으로
설정 화면의 토글은 실제 알림 수신 가능 상태를 나타냄
There was a problem hiding this comment.
@comst19, 이해했습니다! 시스템 알림 권한을 다시 활성화하는 것을 사용자의 명시적인 알림 수신 의사로 간주하여 앱 및 서버 푸시 설정을 자동으로 활성화하는 정책이군요.
이 접근 방식은 사용자가 설정에서 시스템 권한을 다시 켜는 행동 자체를 "알림을 다시 받고 싶다"는 의사 표현으로 해석하는 것이라 합리적입니다. 명확한 설명 감사합니다!
✏️ Learnings added
Learnt from: comst19
Repo: Piece-Puzzly/Piece-Android PR: 203
File: feature/setting/src/main/java/com/puzzle/setting/graph/main/SettingScreen.kt:242-246
Timestamp: 2026-01-10T10:27:20.895Z
Learning: In the Piece-Android project's notification policy: When users re-enable system notification permission (after previously disabling it), the app automatically turns ON both app and server push notification settings, interpreting this as the user's intent to receive notifications again.
Learnt from: comst19
Repo: Piece-Puzzly/Piece-Android PR: 203
File: feature/setting/src/main/java/com/puzzle/setting/graph/main/SettingScreen.kt:242-246
Timestamp: 2026-01-10T10:27:20.895Z
Learning: In the Piece-Android project's SettingScreen.kt and AccessRightsPage.kt: The notification toggle represents the actual notification receivable state (combining system permission and app setting), and when system permission becomes granted, the app automatically enables push notifications through onPushNotificationCheckedChange() callback - this is intentional behavior per the app's notification policy.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
1. ⭐️ 변경된 내용
2. 🖼️ 스크린샷(선택)
3. 💡 알게된 부분
4. 📌 이 부분은 꼭 봐주세요!
문제:
기존 코드에서는 Android 13(Tiramisu) 미만일 경우 permissionList.permissions.find { ... } 내부 조건이 항상 true를 반환하도록 되어 있어, 권한 리스트의 첫 번째 상태를 참조하는 문제가 생기고 그로 인해 UI에 알람 권한 허용 여부가 부정확한 경우가 생김
@tgyuuAn @kkh725
Summary by CodeRabbit
Bug Fixes
New Features
✏️ Tip: You can customize this high-level summary in your review settings.