Fix #20, #24: Add Android system notification support with notification channels #23
Conversation
WalkthroughThis change introduces Android system notification support. It adds notification permission handling, defines a notification channel, implements a notification handler, and integrates permission-aware UI components. The app now requests and manages notification permissions, creates notification channels at startup, and can display notifications with actions, supporting Android 13+ permission requirements. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MainActivity
participant WithNotificationPermission
participant NotificationHandler
participant SystemNotificationManager
User->>MainActivity: Launches app
MainActivity->>WithNotificationPermission: Navigates to notification screen
WithNotificationPermission->>WithNotificationPermission: Checks notification permission
alt Permission not granted
WithNotificationPermission->>User: Shows rationale or denied prompt
User->>WithNotificationPermission: Grants permission or opens settings
end
alt Permission granted
WithNotificationPermission->>NotificationHandler: Triggers notification (when needed)
NotificationHandler->>SystemNotificationManager: Posts notification with PendingIntent
User->>SystemNotificationManager: Taps notification
SystemNotificationManager->>App: Opens app or URL
end
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related PRs
Poem
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
coderabbitai
bot
changed the title
![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 (2)
app/src/main/AndroidManifest.xml (1)
5-6: Consider declaring the permission withuses-permission-sdk-33to avoid confusing pre-T devices.<uses-permission-sdk-33 android:name="android.permission.POST_NOTIFICATIONS" />This prevents the permission from being shown as a normal (always-granted) permission on API < 33, keeping Play-Store policy noise down.
app/src/main/java/com/notifier/app/core/domain/notification/AppNotificationChannel.kt (1)
8-12: Hard-coded strings hurt localisation.
MovedisplayNameanddescriptionintostrings.xmland reference them via@StringResor resource IDs so the app is translatable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/src/main/AndroidManifest.xml(1 hunks)app/src/main/java/com/notifier/app/GithubNotifierApp.kt(1 hunks)app/src/main/java/com/notifier/app/core/domain/notification/AppNotificationChannel.kt(1 hunks)app/src/main/java/com/notifier/app/core/domain/notification/NotificationHandler.kt(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: unit_tests
- GitHub Check: instrumented_tests (29)
- GitHub Check: build
- GitHub Check: ci/bitrise/a7464fec-c4a9-4530-ba90-de9098cb5cd8/pr
🔇 Additional comments (4)
app/src/main/AndroidManifest.xml (1)
6-6:POST_NOTIFICATIONSrequires compileSdk 33+. Make sure your build is already bumped.
If the project is still on compileSdk 32 or lower, the build will fail with attribute not found for this permission.app/src/main/java/com/notifier/app/core/domain/notification/AppNotificationChannel.kt (1)
3-13:entriesrequires Kotlin 1.9 – please verify toolchain.
AppNotificationChannel.entries(used inNotificationHandler) is generated only when compiling with Kotlin 1.9+. Older AGP stacks (< 8.1) ship 1.8.x and will not compile.app/src/main/java/com/notifier/app/GithubNotifierApp.kt (1)
13-17:createNotificationChannels()will crash on API < 26 unless guarded internally.
The call path is unconditional; ensure the method itself performs anSDK_INT >= 26check (see comment inNotificationHandler).app/src/main/java/com/notifier/app/core/domain/notification/NotificationHandler.kt (1)
38-38: Sameentriesconcern as above – compilation depends on Kotlin 1.9.
If upgrading Kotlin is not an option, replace withenumValues<AppNotificationChannel>().
app/src/main/java/com/notifier/app/core/domain/notification/NotificationHandler.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
app/src/main/java/com/notifier/app/core/presentation/NotificationPermissionBanner.kt (2)
32-40: Expose aModifierparameter for easier reuse
WithNotificationPermissionalways wraps its children in a bareColumn, which limits layout flexibility (e.g. alignment, padding).
Add an optionalmodifier: Modifier = Modifierto let callers decide how the wrapper should be placed without introducing another composable layer.-@Composable -fun WithNotificationPermission( - content: @Composable () -> Unit, -) { - Column { +@Composable +fun WithNotificationPermission( + modifier: Modifier = Modifier, + content: @Composable () -> Unit, +) { + Column(modifier = modifier) { NotificationPermissionHandler() content() } }
89-90: Minor nit: inline lambda instead of extraval
val onClick = …is used once; inlining it inTextButton(onClick = …)removes a disposable allocation and shrinks the scope.- val onClick = if (shouldShowRationale) onRequestPermission else onOpenSettings - Column(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/build.gradle.kts(1 hunks)app/src/main/java/com/notifier/app/MainActivity.kt(2 hunks)app/src/main/java/com/notifier/app/core/presentation/NotificationPermissionBanner.kt(1 hunks)gradle/libs.versions.toml(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- app/build.gradle.kts
- gradle/libs.versions.toml
🧰 Additional context used
🧬 Code Graph Analysis (2)
app/src/main/java/com/notifier/app/MainActivity.kt (2)
app/src/main/java/com/notifier/app/core/presentation/NotificationPermissionBanner.kt (1)
WithNotificationPermission(32-40)app/src/main/java/com/notifier/app/notification/presentation/NotificationRoute.kt (1)
NotificationRoute(9-12)
app/src/main/java/com/notifier/app/core/presentation/NotificationPermissionBanner.kt (1)
app/src/main/java/com/notifier/app/ui/theme/Theme.kt (1)
GitHubNotifierTheme(39-61)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: unit_tests
- GitHub Check: instrumented_tests (29)
- GitHub Check: build
- GitHub Check: ci/bitrise/a7464fec-c4a9-4530-ba90-de9098cb5cd8/pr
app/src/main/java/com/notifier/app/core/presentation/NotificationPermissionBanner.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/notifier/app/core/presentation/NotificationPermissionBanner.kt
Outdated
Show resolved
Hide resolved
…nd update permission rationale messages
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/src/main/java/com/notifier/app/core/presentation/NotificationPermissionPrompt.kt (3)
58-66: Avoid unnecessaryLaunchedEffectrestarts by scoping the key more narrowly
permissionState.statusis a data class whose instance is recreated on every recomposition; using it as the sole key forces the effect to cancel & re-launch every frame, even when nothing meaningful has changed.
Limit the key to something stable (e.g.permissionState.status.isGranted) or evenUnit.- LaunchedEffect(permissionState.status) { + LaunchedEffect(permissionState.status.isGranted, hasRequested) {This preserves the single–launch guarantee while reducing churn.
76-85: Harden “open settings” intent against non-activity contexts & missing handlers
LocalContext.currentmay not always be anActivity; starting an explicit settings intent from an application context requires theFLAG_ACTIVITY_NEW_TASK.
Additionally,startActivitycan throw if no handler exists (rare but possible on OEM ROMs).- val intent = Intent( - Settings.ACTION_APPLICATION_DETAILS_SETTINGS, - Uri.fromParts( - /* scheme = */ "package", - /* ssp = */ context.packageName, - /* fragment = */ null - ) - ) - context.startActivity(intent) + runCatching { + Intent( + Settings.ACTION_APPLICATION_DETAILS_SETTINGS, + Uri.fromParts("package", context.packageName, null) + ).apply { + addFlags(Intent.FLAG_ACTIVITY_NEW_TASK) + }.let(context::startActivity) + }Prevents a potential
ActivityNotFoundExceptionand guarantees the launch succeeds from any context.
42-46: Minor UI concern: wrapperColumnmay impose unintended padding/positioningPlacing
NotificationPermissionHandler()abovecontent()inside aColumnwill vertically stack the permission banner and the screen’s root composable, which can shift the entire UI down when the banner is invisible (granted case).If the banner should overlay rather than push content, consider using a
Boxand draw the banner conditionally on top, or move the banner inside aScaffold’s snackbar/host slot.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/src/main/java/com/notifier/app/core/presentation/NotificationPermissionPrompt.kt(1 hunks)app/src/main/res/values/strings.xml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/src/main/res/values/strings.xml
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: instrumented_tests (29)
- GitHub Check: unit_tests
- GitHub Check: build
- GitHub Check: ci/bitrise/a7464fec-c4a9-4530-ba90-de9098cb5cd8/pr
🔇 Additional comments (1)
app/src/main/java/com/notifier/app/core/presentation/NotificationPermissionPrompt.kt (1)
1-160: Overall implementation looks solidKudos on the clear separation of concerns, Material 3 theming, and preview support. The runtime logic correctly respects API < 33 devices and handles the three notification-permission scenarios.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/src/main/java/com/notifier/app/core/presentation/WithNotificationPermission.kt (3)
91-102: Avoid silently swallowingstartActivityfailures – at least log the exception
runCatching { … }.let {}suppresses every throwable, making runtime problems hard to diagnose (e.g.,ActivityNotFoundException, malformedIntent).
Capture the failure and record it (Logcat / Crashlytics) so that unexpected issues surface during QA and in production.- runCatching { + runCatching { … - } + }.onFailure { Log.e("NotificationPermission", "Unable to open app settings", it) }
71-80: Guard theLaunchedEffectwith a stable key to avoid recomposition churn
Using the wholepermissionState.statusobject as the key recreates the coroutine whenever any internal field instance changes.
A lighter key such aspermissionState.status.isGranted(or evenUnit) combined with the explicithasRequestedflag is sufficient and reduces unnecessary launches:- LaunchedEffect(permissionState.status) { + LaunchedEffect(permissionState.status.isGranted) {This keeps the behaviour identical while avoiding redundant coroutine restarts.
50-54: ConsiderBox+alignfor overlay prompts
Placing the prompt and screen content in aColumnmeans the permission banner always consumes vertical space and may push critical UI elements off-screen.
Wrapping them in aBoxand aligning the prompt toAlignment.TopCenterlets the banner overlay the screen non-intrusively:- Column { - NotificationPermissionHandler() - content() - } + Box { + content() + NotificationPermissionHandler() + }This mirrors common in-app banner patterns (e.g., Snackbars) and avoids layout shifts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/main/java/com/notifier/app/core/presentation/WithNotificationPermission.kt(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
app/src/main/java/com/notifier/app/core/presentation/WithNotificationPermission.kt (1)
app/src/main/java/com/notifier/app/ui/theme/Theme.kt (1)
GitHubNotifierTheme(39-61)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: instrumented_tests (29)
- GitHub Check: build
- GitHub Check: ci/bitrise/a7464fec-c4a9-4530-ba90-de9098cb5cd8/pr
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
.github/workflows/test.yml (1)
37-37: Consider disabling fail-fast for the matrixIf one API level fails early, the other job is cancelled by default. Explicitly adding
strategy: fail-fast: falsekeeps both results and shortens the feedback loop when only one level flakes.
app/src/main/java/com/notifier/app/core/presentation/notification/rememberNotificationPermissionState.kt (1)
25-38: Return value isn’tremember-stable across recompositionsCreating a new anonymous object on every recomposition can invalidate
remember { … }callers or cause unnecessary recompositions higher up the tree. Wrap the object inrememberso its reference remains stable while its delegated values still update.- return object : NotificationPermissionState { + return remember(state) { + object : NotificationPermissionState { override val shouldShowRationale: Boolean get() = state.status.shouldShowRationale override val isGranted: Boolean get() = state.status.isGranted override fun requestPermission() { state.launchPermissionRequest() } - } + } + }app/src/androidTest/java/com/notifier/app/core/presentation/notification/WithNotificationPermissionTest.kt (1)
31-38: Hard-coded strings couple tests to UI copyUsing literal text makes the tests fragile when wording changes in
strings.xml. Consider injecting the strings viastringResource(R.string.xxx)in the composable and referencing the same IDs in tests (e.g., by reading them from the context) to decouple tests from copy tweaks.app/src/main/java/com/notifier/app/core/presentation/notification/WithNotificationPermission.kt (4)
45-57: Expose amodifierfor greater composability flexibilityThe wrapper composable hard-codes an internal
Columnwithout exposing aModifier. Call-sites cannot adjust padding, scrolling behaviour, or test tags. A tiny signature change preserves binary compatibility while unlocking far more use-cases.-@Composable -fun WithNotificationPermission( - permissionState: NotificationPermissionState? = null, - content: @Composable () -> Unit, -) { - Column { +@Composable +fun WithNotificationPermission( + modifier: Modifier = Modifier, + permissionState: NotificationPermissionState? = null, + content: @Composable () -> Unit, +) { + Column(modifier = modifier) {
88-101: Avoid swallowing failures when opening Settings
runCatching { … }hides exceptions and gives no signal if theIntentcannot be resolved (rare on AOSP-only devices/emulators).
Logging (or at least printing) the error makes future bug reports diagnosable.-runCatching { +runCatching { … startActivity(…) -} +}.onFailure { throwable -> + Log.e("NotificationPermission", "Unable to open app settings", throwable) +}(Replace
Logwith your preferred logger.)
122-134:message/actionButtonTextrecomputed on every recompositionBoth derived strings are pure functions of
shouldShowRationale; extracting them withremember(shouldShowRationale)avoids unnecessary work and allocations—particularly useful when this prompt sits in an always-visible scaffold.val (message, actionButtonText) = remember(shouldShowRationale) { if (shouldShowRationale) { stringResource(R.string.notification_permission_rationale_message) to stringResource(R.string.notification_permission_allow_button_text) } else { stringResource(R.string.notification_permission_denied_message) to stringResource(R.string.notification_permission_settings_button_text) } }
181-195: Add a preview for the fullWithNotificationPermissionwrapperCurrent previews cover only the prompt. A snapshot of the wrapper simplifies visual regression checks and demonstrates default styling once the modifier change (see above) is applied.
Consider:@PreviewLightDark @Composable private fun WithNotificationPermissionPreview() { GitHubNotifierTheme { WithNotificationPermission( permissionState = FakeNotificationPermissionState( granted = false, shouldShowRationale = true ) ) { Text("Main content goes here") } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/test.yml(1 hunks)app/src/androidTest/java/com/notifier/app/core/presentation/notification/FakeNotificationPermissionState.kt(1 hunks)app/src/androidTest/java/com/notifier/app/core/presentation/notification/WithNotificationPermissionTest.kt(1 hunks)app/src/main/java/com/notifier/app/MainActivity.kt(2 hunks)app/src/main/java/com/notifier/app/core/presentation/notification/NotificationPermissionState.kt(1 hunks)app/src/main/java/com/notifier/app/core/presentation/notification/WithNotificationPermission.kt(1 hunks)app/src/main/java/com/notifier/app/core/presentation/notification/rememberNotificationPermissionState.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/com/notifier/app/MainActivity.kt
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: ci/bitrise/a7464fec-c4a9-4530-ba90-de9098cb5cd8/pr
🔇 Additional comments (3)
.github/workflows/test.yml (1)
37-37: Expanded test matrix to API 33 looks goodCovering Android 13 in CI aligns with the new notification-permission code and should prevent regressions.
app/src/androidTest/java/com/notifier/app/core/presentation/notification/FakeNotificationPermissionState.kt (1)
12-18: Lean, purposeful test doubleThe fake cleanly fulfills the interface without extra baggage—nice.
app/src/main/java/com/notifier/app/core/presentation/notification/NotificationPermissionState.kt (1)
9-30: Solid abstraction enables decoupled UI & logicThe interface is minimal yet sufficient for real and fake implementations—good design.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/test.yml (1)
48-52: DRY up Java setup stepsThe JDK setup for both
unit_testsandinstrumented_testsis identical. Consider using a YAML anchor or a reusable workflow to avoid duplication and ease future updates.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/test.yml(3 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/test.yml
38-38: expecting a single ${{...}} expression or array value for matrix variations, but found plain text node
(syntax-check)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: ci/bitrise/a7464fec-c4a9-4530-ba90-de9098cb5cd8/pr
🔇 Additional comments (3)
.github/workflows/test.yml (3)
57-58: Approve: gradlew execution permissionGranting execute permissions on
gradlewbefore any invocations ensures the wrapper can run correctly in this job.
73-79: Approve: AVD snapshot creation settingsThe added
target: google_apis,arch: x86_64,force-avd-creation: true,disable-animations: true, and snapshot script correctly prepare and cache emulator images for both API levels.
85-86: Approve: emulator target and architectureSpecifying the same
targetandarchin the test run ensures consistency with the snapshot creation step.
… state retention and add error logging for app settings access failure
… install required system images
theMr17
changed the title
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/src/main/java/com/notifier/app/MainActivity.kt (1)
95-99: Permission banner timing issue remains unaddressed.The integration of
WithNotificationPermissionis architecturally sound, but the previously identified timing issue where the permission banner appears before the system dialog still needs to be resolved to avoid user confusion.
🧹 Nitpick comments (4)
app/src/main/AndroidManifest.xml (1)
5-7: Permission declaration is correct—consider addingtools:targetApi
You’ve correctly added the POST_NOTIFICATIONS permission for Android 13+. To suppress lint warnings on lower API levels, you can addtools:targetApi="33"on this line:<uses-permission android:name="android.permission.POST_NOTIFICATIONS" tools:targetApi="33" />app/build.gradle.kts (1)
104-107: Add accompanist-permissions to instrumentation tests
Since your UI tests underandroidTestinvokeWithNotificationPermission, you’ll need the Accompanist permissions library on that classpath. Consider adding:androidTestImplementation(libs.accompanist.permissions)to ensure your instrumentation tests compile and run without missing-dependency errors.
.github/workflows/test.yml (1)
54-65: Consolidate duplicated setup steps
You’ve duplicated the JDK setup andchmod +x gradlewsteps in both jobs. Consider extracting these into a reusable composite action or a shared job definition to adhere to DRY principles.app/src/androidTest/java/com/notifier/app/core/presentation/notification/WithNotificationPermissionTest.kt (1)
34-44: Consider using string resources in tests for consistency.The tests use hardcoded strings that should match the actual string resources. Consider extracting these into a test utility or using the string resources directly to prevent potential inconsistencies if the actual strings change.
Example approach:
// Instead of hardcoded strings, use: composeTestRule .onNodeWithText( - "To deliver GitHub notifications like pull requests, issues, and mentions, " + - "we need notification access. Please allow it." + context.getString(R.string.notification_permission_rationale_message) ) .assertIsDisplayed()Also applies to: 66-77
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.github/workflows/test.yml(2 hunks)app/build.gradle.kts(1 hunks)app/src/androidTest/java/com/notifier/app/core/presentation/notification/FakeNotificationPermissionState.kt(1 hunks)app/src/androidTest/java/com/notifier/app/core/presentation/notification/WithNotificationPermissionTest.kt(1 hunks)app/src/main/AndroidManifest.xml(1 hunks)app/src/main/java/com/notifier/app/GithubNotifierApp.kt(1 hunks)app/src/main/java/com/notifier/app/MainActivity.kt(2 hunks)app/src/main/java/com/notifier/app/core/domain/notification/AppNotificationChannel.kt(1 hunks)app/src/main/java/com/notifier/app/core/domain/notification/NotificationHandler.kt(1 hunks)app/src/main/java/com/notifier/app/core/presentation/notification/NotificationPermissionState.kt(1 hunks)app/src/main/java/com/notifier/app/core/presentation/notification/WithNotificationPermission.kt(1 hunks)app/src/main/java/com/notifier/app/core/presentation/notification/rememberNotificationPermissionState.kt(1 hunks)app/src/main/res/values/strings.xml(1 hunks)gradle/libs.versions.toml(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
app/src/main/java/com/notifier/app/core/domain/notification/NotificationHandler.kt (1)
Learnt from: theMr17
PR: theMr17/github-notifier#23
File: app/src/main/java/com/notifier/app/core/domain/notification/NotificationHandler.kt:37-53
Timestamp: 2025-06-14T12:33:29.399Z
Learning: The github-notifier Android app has minSdkVersion set to 26, so API-level guards for Android 8.0+ features like NotificationChannel are not required.
🔇 Additional comments (18)
gradle/libs.versions.toml (1)
21-21: Accompanist permissions dependency declared correctly
The newaccompanistPermissions = "0.37.3"version and correspondingaccompanist-permissionslibrary entry align with the project’s version catalog format and support the runtime permission flow.Also applies to: 55-55
app/src/main/res/values/strings.xml (1)
17-20: Notification permission strings added
The rationale, denial message, and button labels for the notification-permission dialog are well-defined and follow the existing naming conventions..github/workflows/test.yml (4)
35-44: Expanded instrumented test matrix configuration is correct
Theincludeblock now covers API levels 29 & 33 with x86/x86_64, andfail-fastis correctly placed outsidematrix. This ensures broad coverage without syntax errors.
73-74: AVD cache key enhancement approved
Including both${{ matrix.api-level }}and${{ matrix.arch }}in the cache key is a solid approach to avoid collisions and ensure correct snapshot reuse per device config.
80-85: AVD creation parameters align with requirements
Specifyingarch,target: google_apis,force-avd-creation, and disabling animations ensures consistent emulator snapshots and fast cold boots across matrix entries.
91-96: Run-in-AVD test execution is sound
Passingapi-level,arch,target, and calling./gradlew connectedDebugAndroidTest --daemonmatches the snapshot creation step and leverages the matrix correctly for instrumented tests.app/src/main/java/com/notifier/app/core/domain/notification/AppNotificationChannel.kt (1)
11-33: Well-designed notification channel enum.The enum structure is clean and follows Android notification channel best practices. The comprehensive documentation and extensible design will facilitate future channel additions.
app/src/androidTest/java/com/notifier/app/core/presentation/notification/FakeNotificationPermissionState.kt (1)
12-18: Clean test double implementation.The fake implementation properly serves its testing purpose with clear documentation and appropriate no-op behavior for the permission request method.
app/src/main/java/com/notifier/app/core/presentation/notification/NotificationPermissionState.kt (1)
9-30: Well-designed permission state interface.The interface provides a clean abstraction for notification permission handling with comprehensive documentation. The three-member design captures all essential aspects of Android permission state management.
app/src/main/java/com/notifier/app/GithubNotifierApp.kt (1)
9-18: Proper notification channel initialization.The implementation correctly initializes notification channels in the Application's
onCreate()method, ensuring they're available throughout the app's lifecycle. The dependency injection pattern is properly implemented.app/src/main/java/com/notifier/app/core/presentation/notification/rememberNotificationPermissionState.kt (1)
23-42: Well-implemented permission state management.The implementation correctly handles Android 13+ notification permissions using the Accompanist library. The
@RequiresApiannotation, proper use ofremember()for state management, and clean delegation to the underlying permission state follow best practices.app/src/main/java/com/notifier/app/core/domain/notification/NotificationHandler.kt (2)
37-65: Solid notification implementation with proper Android patterns.The
showNotificationmethod correctly usesPendingIntent.FLAG_UPDATE_CURRENT | FLAG_IMMUTABLEflags and follows standard notification building practices. The implementation properly handles notification display with appropriate priority and auto-cancel behavior.
73-89: Channel creation correctly implemented for the app's minSdkVersion.Based on the retrieved learning that this app has
minSdkVersion = 26, theNotificationChannelcreation doesn't require API level guards since all supported devices will have this API available.app/src/main/java/com/notifier/app/core/presentation/notification/WithNotificationPermission.kt (4)
45-58: Clean conditional permission handling for Android 13+.The implementation correctly checks the API level and only applies permission handling on Android 13+ devices. The structure allows the main content to always be displayed regardless of permission state.
75-80: Smart automatic permission request logic.The
LaunchedEffectcorrectly implements the one-time automatic permission request when the permission hasn't been requested and no rationale is needed. UsinghasRequestedwithrememberSaveableensures proper state persistence across configuration changes.
90-108: Robust error handling for settings navigation.The
runCatchingblock properly handles potential failures when opening app settings, with appropriate logging. This prevents crashes if the settings intent cannot be resolved.
124-167: Well-designed adaptive permission prompt UI.The prompt correctly adapts its message and action based on the
shouldShowRationalestate, providing appropriate user guidance for both scenarios. The Material 3 theming and layout structure follow good design practices.app/src/androidTest/java/com/notifier/app/core/presentation/notification/WithNotificationPermissionTest.kt (1)
14-112: Comprehensive test coverage for permission states.The tests thoroughly cover all permission scenarios (granted, denied with rationale, permanently denied) and properly verify UI behavior. The use of
@SdkSuppress(minSdkVersion = 33)correctly matches the Android 13+ requirement, and theFakeNotificationPermissionStateprovides good test isolation.
Summary by CodeRabbit
New Features
Improvements
Tests
Chores
Issue Reference
Essential Checklist