fix: implement notification channels on android and refactor interface#229
Conversation
Reviewer's GuideImplements Android notification channels and a configurable notification options interface, while making MobileNotificationService more testable and adding unit tests for channel creation, channel selection, and existing task completion flows. Sequence diagram for showing a notification with Android channels and optionssequenceDiagram
actor AppLogic
participant MobileNotificationService
participant FlutterLocalNotificationsPlugin
participant AndroidFlutterLocalNotificationsPlugin as AndroidPlugin
AppLogic->>MobileNotificationService: show(title, body, payload, id, options)
MobileNotificationService->>MobileNotificationService: isEnabled()
alt notifications disabled
MobileNotificationService-->>AppLogic: return
else enabled
MobileNotificationService->>MobileNotificationService: _checkPermission()
alt permission not granted
MobileNotificationService-->>AppLogic: return
else permission granted
MobileNotificationService->>MobileNotificationService: determine effectiveChannelId and effectiveChannelName
MobileNotificationService->>FlutterLocalNotificationsPlugin: show(effectiveId, title, body, NotificationDetails(androidDetails))
FlutterLocalNotificationsPlugin-->>MobileNotificationService: done
MobileNotificationService-->>AppLogic: complete
end
end
AppLogic->>MobileNotificationService: init()
MobileNotificationService->>FlutterLocalNotificationsPlugin: initialize(InitializationSettings)
MobileNotificationService->>MobileNotificationService: _createNotificationChannels()
MobileNotificationService->>FlutterLocalNotificationsPlugin: resolvePlatformSpecificImplementation()
FlutterLocalNotificationsPlugin-->>MobileNotificationService: AndroidPlugin
MobileNotificationService->>AndroidPlugin: createNotificationChannel(taskChannel)
MobileNotificationService->>AndroidPlugin: createNotificationChannel(habitChannel)
AndroidPlugin-->>MobileNotificationService: channels created
MobileNotificationService-->>AppLogic: init complete
Class diagram for updated notification services and optionsclassDiagram
direction LR
class INotificationService {
<<interface>>
+Future~void~ init()
+Future~void~ show(title String, body String, payload String, id int, options NotificationOptions)
+Future~void~ clearAll()
+Future~bool~ isEnabled()
+Future~void~ handleNotificationTaskCompletion(String taskId)
+Future~void~ handleNotificationHabitCompletion(String habitId)
+Future~bool~ checkPermissionStatus()
+Future~bool~ requestPermission()
}
class NotificationOptions {
+String actionButtonText
+String channelId
+NotificationOptions(actionButtonText String, channelId String)
}
class MobileNotificationService {
-Mediator _mediator
-FlutterLocalNotificationsPlugin _flutterLocalNotifications
-bool _isAndroid
-bool _isIOS
+MobileNotificationService(Mediator mediator, FlutterLocalNotificationsPlugin flutterLocalNotifications, bool isAndroid, bool isIOS)
+Future~void~ init()
-Future~void~ _createNotificationChannels()
+Future~void~ show(title String, body String, payload String, id int, options NotificationOptions)
-Future~bool~ _checkPermission()
+Future~bool~ checkPermissionStatus()
+Future~bool~ requestPermission()
}
class DesktopNotificationService {
+Future~void~ init()
+Future~void~ show(title String, body String, payload String, id int, options NotificationOptions)
+Future~void~ clearAll()
+Future~bool~ isEnabled()
+Future~void~ handleNotificationTaskCompletion(String taskId)
+Future~void~ handleNotificationHabitCompletion(String habitId)
+Future~bool~ checkPermissionStatus()
+Future~bool~ requestPermission()
}
class FlutterLocalNotificationsPlugin {
+Future~bool~ initialize(InitializationSettings settings)
+Future~void~ show(int id, String title, String body, NotificationDetails details)
+T resolvePlatformSpecificImplementation()
}
class AndroidFlutterLocalNotificationsPlugin {
+Future~bool~ requestNotificationsPermission()
+Future~void~ createNotificationChannel(AndroidNotificationChannel channel)
+Future~NotificationAppLaunchDetails~ getNotificationAppLaunchDetails()
}
class AndroidNotificationChannel {
+String id
+String name
+Importance importance
+bool playSound
+bool enableVibration
+bool enableLights
}
class AndroidAppConstants_notificationChannels {
+String taskChannelId
+String taskChannelName
+String habitChannelId
+String habitChannelName
}
INotificationService <|.. MobileNotificationService
INotificationService <|.. DesktopNotificationService
MobileNotificationService --> FlutterLocalNotificationsPlugin : uses
MobileNotificationService --> AndroidFlutterLocalNotificationsPlugin : resolves
MobileNotificationService --> AndroidNotificationChannel : creates
MobileNotificationService --> AndroidAppConstants_notificationChannels : reads ids and names
MobileNotificationService --> NotificationOptions : reads options
DesktopNotificationService --> NotificationOptions : reads options
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary of ChangesHello @ahmet-cetinkaya, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves critical notification delivery issues on modern Android devices by implementing explicit notification channels. It also significantly refactors the notification service interface and its mobile implementation to enhance maintainability, support platform-specific configurations more cleanly, and drastically improve the testability of notification-related logic through dependency injection. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
MobileNotificationService.requestPermission, the early-return condition still usesPlatform.isIOSinstead of the injected_isIOSflag, which undermines the testability / platform override pattern you introduced elsewhere; consider using the injected flags consistently for all platform checks. - In
MobileNotificationService.show, you unconditionally deriveeffectiveChannelId/effectiveChannelNamefromAndroidAppConstantsand always constructAndroidNotificationDetails; it would be cleaner (and safer for future platforms) to guard this Android-specific logic behind_isAndroidand keep the non-Android path free of Android constants/types.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `MobileNotificationService.requestPermission`, the early-return condition still uses `Platform.isIOS` instead of the injected `_isIOS` flag, which undermines the testability / platform override pattern you introduced elsewhere; consider using the injected flags consistently for all platform checks.
- In `MobileNotificationService.show`, you unconditionally derive `effectiveChannelId`/`effectiveChannelName` from `AndroidAppConstants` and always construct `AndroidNotificationDetails`; it would be cleaner (and safer for future platforms) to guard this Android-specific logic behind `_isAndroid` and keep the non-Android path free of Android constants/types.
## Individual Comments
### Comment 1
<location> `src/lib/infrastructure/mobile/features/notification/mobile_notification_service.dart:33-34` </location>
<code_context>
+ _isAndroid = isAndroid ?? Platform.isAndroid,
+ _isIOS = isIOS ?? Platform.isIOS;
@override
Future<void> init() async {
</code_context>
<issue_to_address>
**issue (bug_risk):** Use `_isIOS` in `checkPermissionStatus` instead of `Platform.isIOS` for consistency with the injectable platform flags.
Since `checkPermissionStatus` now has injectable `_isAndroid`/`_isIOS` flags and the Android branch already uses `_isAndroid`, the iOS branch should use `_isIOS` instead of `Platform.isIOS`. Otherwise, overrides of `_isIOS` (e.g., in tests) won’t affect this code path, making behavior inconsistent with `_checkPermission` and `requestPermission`. Please switch the iOS check to `_isIOS` for consistent, predictable behavior when forcing platform flags.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Code Review
This pull request successfully implements Android notification channels and refactors the INotificationService interface to use a NotificationOptions object. This significantly improves the maintainability and clarity of the notification service, especially for handling platform-specific details. The introduction of dependency injection for platform flags in MobileNotificationService is a great step towards improving testability, and the new unit tests adequately cover the added functionality. Overall, the changes are well-structured and address the stated goals effectively.
| Future<bool> checkPermissionStatus() async { | ||
| if (!PlatformUtils.isMobile) { | ||
| // Override platform check if flags are forced (testing) | ||
| if (!_isAndroid && !_isIOS && !PlatformUtils.isMobile) { |
There was a problem hiding this comment.
The condition !_isAndroid && !_isIOS && !PlatformUtils.isMobile seems a bit redundant. If _isAndroid and _isIOS are false, and PlatformUtils.isMobile is also false, then it's already a non-mobile platform. You could simplify this to !PlatformUtils.isMobile if _isAndroid and _isIOS are guaranteed to be false when PlatformUtils.isMobile is false, or clarify the specific testing scenario where this full condition is necessary.
| if (!_isAndroid && !_isIOS && !PlatformUtils.isMobile) { | |
| if (!PlatformUtils.isMobile && !_isAndroid && !_isIOS) { |
| Future<bool> requestPermission() async { | ||
| if (!PlatformUtils.isMobile && !Platform.isIOS) { | ||
| // Override platform check if flags are forced (testing) | ||
| if (!_isAndroid && !_isIOS && !PlatformUtils.isMobile && !Platform.isIOS) { |
There was a problem hiding this comment.
Similar to the checkPermissionStatus method, the condition !_isAndroid && !_isIOS && !PlatformUtils.isMobile && !Platform.isIOS is quite verbose. Given that _isIOS is already a flag for Platform.isIOS, the last !Platform.isIOS part is redundant. Consider simplifying this condition for better readability, while still maintaining the testability override.
| if (!_isAndroid && !_isIOS && !PlatformUtils.isMobile && !Platform.isIOS) { | |
| if (!PlatformUtils.isMobile && !_isAndroid && !_isIOS) { |
PR Review: #229 - fix: implement notification channels on android and refactor interfaceOverall AssessmentThis PR implements Android notification channels correctly and follows Clean Architecture principles. The import data migration and command handling demonstrate excellent error handling practices. However, there are several critical issues in the notification services that need to be addressed before merging. Recommendation: Request changes - Fix critical issues first Critical Issues (Must Fix)1. Silent Failures in
|
…service - Fix isEnabled() to return false on error instead of true (safer default) - Add error handling to _createNotificationChannels() with proper logging - Add stackTrace parameter to all Logger.error calls - Use _isIOS consistently in checkPermissionStatus() for testability - Simplify redundant platform checks in checkPermissionStatus() and requestPermission() - Guard Android-specific notification logic behind _isAndroid in show() - Add iOS permission check in checkPermissionStatus() These changes improve error handling, consistency, and testability of the mobile notification service.
This PR fixes notification issues on modern Android devices (Galaxy S24+) by explicitly creating notification channels. It also refactors the INotificationService interface to use a NotificationOptions object for better maintainability and clean code.
Changes:
Summary by Sourcery
Implement Android notification channels and refactor the notification service API for better platform handling and testability.
New Features:
Enhancements:
Tests: