Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Summary by CodeRabbit
Summary by CodeRabbit
WalkthroughThis update restructures localization files, enhances notification permission handling using the Changes
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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
lib/common/composition/dialog/custom_dialog.dart (1)
85-87: Review the removal of null safety check.The content now directly uses
message!without checking if it's null first. This is safe given the required parameter change above, but creates a tight coupling between the constructor requirement and this implementation.Consider adding an assertion for extra safety:
SingleChildScrollView( - child: CustomText(text: message!, style: context.textTheme.bodyMedium), + child: CustomText(text: message!, style: context.textTheme.bodyMedium), ),The current implementation is correct, but the assertion would make the requirement more explicit.
lib/features/debug_tools/page/actions/debug_tools_actions_page_content.dart (1)
52-64: Optimize the permission checking logic for better efficiency.The current implementation works but can be optimized to avoid unnecessary permission requests when permissions are already granted or permanently denied.
Consider this more efficient approach:
- onPressed: () async { - final isPermanentlyDenied = await NotificationsService.isPermissionsPermanentlyDenied(); - final isDenied = isPermanentlyDenied || !(await NotificationsService.requestPermission()); - if (isDenied && context.mounted) { - await CustomNotificationPermissionDeniedDialog.show(context); - return; - } else if (context.mounted) { - await CustomSnackbarSuccess(context: context, message: 'Permission granted').show(); - } - }, + onPressed: () async { + final isPermanentlyDenied = await NotificationsService.isPermissionsPermanentlyDenied(); + + if (isPermanentlyDenied) { + if (context.mounted) { + await CustomNotificationPermissionDeniedDialog.show(context); + } + return; + } + + final isGranted = await NotificationsService.requestPermission(); + if (!isGranted && context.mounted) { + await CustomNotificationPermissionDeniedDialog.show(context); + } else if (context.mounted) { + await CustomSnackbarSuccess(context: context, message: 'Permission granted').show(); + } + },This avoids calling
requestPermission()when permissions are permanently denied and makes the logic flow clearer.lib/common/composition/dialog/custom_notification_permission_denied_dialog.dart (2)
7-9: Improve documentation clarity.The class documentation should specify what the
Future<bool>return value represents and when each boolean value is returned.-/// This method can be used to display a dialog to the user when the permission is denied. -/// It will show a dialog with a message and a button to open the app settings. +/// Dialog for notifying users when notification permissions are denied. +/// Provides options to open app settings or cancel the request. +/// +/// Returns `true` if the user selected the positive action (Grant Access), +/// `false` if the user selected the neutral action (Cancel) or dismissed the dialog.
10-20: Consider adding error handling for app settings navigation.The
openAppSettings()call might fail on some platforms or configurations. Consider wrapping it in a try-catch block to handle potential exceptions gracefully.static Future<bool> show(BuildContext context) async => await CustomDialog.alert( context: context, title: context.locale.notificationsPermissionDeniedDialogTitle, message: context.locale.notificationsPermissionDeniedDialogText, positiveActionTitle: context.locale.notificationsPermissionDeniedDialogPositiveAction, - positiveAction: openAppSettings, + positiveAction: () async { + try { + await openAppSettings(); + } catch (e) { + // Handle potential failures silently or log them + // The dialog will still return the user's choice + } + }, neutralActionTitle: context.locale.notificationsPermissionDeniedDialogNeutralAction, ).show() ?? false;assets/localization/app_en.arb (1)
28-31: Consider adding a rationale message for better UX.The permission strings cover the basic dialog needs, but consider adding a rationale message that explains why the app needs notification permissions. This can improve user understanding and permission grant rates.
Add an additional string for permission rationale:
"@@x---PERMISSIONS_HANDLING": 0, +"notificationsPermissionRationaleText": "Notifications help you stay updated with important information and timely alerts from the app.", "notificationsPermissionDeniedDialogTitle": "Notification Permission", "notificationsPermissionDeniedDialogText": "Please grant the notifications permission to receive updates.", "notificationsPermissionDeniedDialogPositiveAction": "Grant Access", "notificationsPermissionDeniedDialogNeutralAction": "Cancel",lib/common/provider/notifications_service.dart (2)
16-20: Fix naming inconsistency.The method name uses plural "Permissions" while the similar method
isPermissionsPermanentlyDenieduses singular "Permission". Consider standardizing the naming.-/// Check whether user has at least any Notification permission allowed -static Future<bool> isPermissionsGranted() async { +/// Check whether user has notification permission granted +static Future<bool> isPermissionGranted() async {
22-23: Fix naming inconsistency.Update this method name to match the singular form for consistency.
-/// Check whether user has at least any Notification permission denied -static Future<bool> isPermissionsPermanentlyDenied() async => Permission.notification.isPermanentlyDenied; +/// Check whether user has notification permission permanently denied +static Future<bool> isPermissionPermanentlyDenied() async => Permission.notification.isPermanentlyDenied;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between afe9572 and 9b94191c4f731085bd9adad7638952886ae06754.
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
README.md(1 hunks)assets/localization/app_en.arb(2 hunks)flutter.code-workspace(1 hunks)ios/Podfile(1 hunks)lib/common/composition/dialog/custom_alert_dialog.dart(0 hunks)lib/common/composition/dialog/custom_dialog.dart(4 hunks)lib/common/composition/dialog/custom_notification_permission_denied_dialog.dart(1 hunks)lib/common/provider/firebase_messaging_service.dart(0 hunks)lib/common/provider/notifications_service.dart(2 hunks)lib/core/database/shared_preferences.dart(0 hunks)lib/features/debug_tools/page/actions/debug_tools_actions_page_content.dart(3 hunks)lib/features/debug_tools/page/popups/debug_tools_list_dialog.dart(2 hunks)lib/features/debug_tools/page/popups/debug_tools_popups_page_content.dart(2 hunks)makefile(2 hunks)project_setup/README.md(1 hunks)pubspec.yaml(1 hunks)windows/flutter/generated_plugin_registrant.cc(2 hunks)windows/flutter/generated_plugins.cmake(1 hunks)
💤 Files with no reviewable changes (3)
- lib/core/database/shared_preferences.dart
- lib/common/composition/dialog/custom_alert_dialog.dart
- lib/common/provider/firebase_messaging_service.dart
🔇 Additional comments (17)
pubspec.yaml (1)
81-81:Details
✅ Verification successful
Verify the package version and check for security advisories.
The addition of
permission_handler: 12.0.0+1aligns with the PR's notification permission handling objectives. However, please ensure this version is current and secure.
🌐 Web query:
What is the latest version of permission_handler package on pub.dev and are there any known security issues with version 12.0.0+1?💡 Result:
The latest version of the
permission_handlerpackage on pub.dev is 12.0.0+1, published 27 days ago. (pub.dev)This release includes updates to the README documentation, changing the
compileSdkVersionfrom 33 to 35, and updates the example project'sIPHONEOS_DEPLOYMENT_TARGETfrom 11.0 to 12.0. (pub.dev)As of now, there are no known security issues associated with version 12.0.0+1. The changelog does not mention any security vulnerabilities or fixes in this release. (pub.dev)
For the most current information on security issues, it's advisable to monitor the GitHub repository for any reported vulnerabilities or updates.
Citations:
- 1: https://pub.dev/packages/permission_handler/changelog?utm_source=openai
- 2: https://pub.dev/packages/permission_handler/changelog?utm_source=openai
- 3: https://pub.dev/packages/permission_handler/changelog?utm_source=openai
permission_handler is up-to-date and secure
The
permission_handler: 12.0.0+1dependency you’ve added is the latest release (published 27 days ago) and there are no reported security vulnerabilities in this version. You can proceed with this update.project_setup/README.md (1)
31-31: LGTM! Documentation improvement.Good catch on the typo correction from "Aplication" to "Application". This enhances the documentation quality.
lib/common/composition/dialog/custom_dialog.dart (3)
7-7: LGTM! Class rename improves clarity.The rename from
CustomDialogWrappertoCustomDialogis more descriptive and aligns with typical Flutter naming conventions.
124-129: LGTM! Excellent API improvement.The new
show()method provides a cleaner, more fluent API for displaying dialogs. This eliminates the need for callers to manually callshowDialog()and improves code readability.
11-11:Details
✅ Verification successful
Verify impact of making message parameter required.
Making the
messageparameter required in thealertconstructor could break existing code that previously passed null. Please ensure all callers have been updated accordingly.
🏁 Script executed:
#!/bin/bash # Search for usages of CustomDialog.alert to verify message parameter usage rg -A 3 "CustomDialog\.alert" --type dartLength of output: 1409
All
CustomDialog.alertcalls include amessageargument
A search forCustomDialog.alertshows every usage passing amessage, so making it required does not introduce a breaking change.• lib/features/debug_tools/page/popups/debug_tools_popups_page_content.dart
• lib/common/composition/dialog/custom_notification_permission_denied_dialog.dartflutter.code-workspace (1)
223-228: LGTM! Developer experience improvement.The addition of syntax highlighting for
@@x---section markers will help developers navigate localization files more easily. The red color choice makes these markers visually distinct.README.md (1)
175-175: LGTM! Documentation correction looks good.The change from
make build_runnertomake watchaligns with the makefile command and provides accurate instructions for developers working with code generation.windows/flutter/generated_plugins.cmake (1)
8-8: Generated file update for Windows permission handling.The addition of
permission_handler_windowsto the plugin list is correct and necessary for Windows platform support. This is a generated file that automatically includes the plugin in the build system.lib/features/debug_tools/page/popups/debug_tools_popups_page_content.dart (2)
10-10: Import updated for dialog refactoring.The import change from
custom_alert_dialog.darttocustom_dialog.dartreflects the consolidation of dialog components into a unifiedCustomDialogclass.
28-35:Details
✅ Verification successful
Dialog usage updated to new API.
The change from
CustomAlertDialogtoCustomDialog.alertmaintains the same parameters and functionality while using the refactored dialog API. The.show()method call suggests the new implementation returns a dialog object that needs to be explicitly shown.Verify that the new
CustomDialog.alertimplementation maintains the same behavior as the previousCustomAlertDialog:
🏁 Script executed:
#!/bin/bash # Description: Verify the CustomDialog.alert implementation matches expected behavior # Search for CustomDialog class definition to understand the new API ast-grep --pattern $'class CustomDialog { $$$ static $_($$$) { $$$ } $$$ }' # Search for any remaining usage of CustomAlertDialog to ensure complete migration rg "CustomAlertDialog" -A 3Length of output: 116
I didn’t find any
CustomDialogdefinitions or usages ofCustomAlertDialog. Let’s search more broadly:
🏁 Script executed:
#!/bin/bash set -e echo "Looking for CustomDialog class definitions…" rg "class CustomDialog" -A5 echo "Looking for CustomDialog.alert(...) implementation…" rg -n "CustomDialog\.alert" -C3 echo "Looking for any remaining CustomAlertDialog references…" rg -n "CustomAlertDialog" -A3Length of output: 3367
Verification Complete: Migration to
CustomDialog.alertIs CorrectI confirmed the following:
CustomDialog.alertis defined inlib/common/composition/dialog/custom_dialog.dartwith the same parameters as the old API.- It exposes a
.show()method for displaying the dialog.- There are no remaining references to
CustomAlertDialogin the codebase.All dialog usages have been migrated successfully.
windows/flutter/generated_plugin_registrant.cc (2)
11-11: Permission handler plugin header included.The inclusion of the
permission_handler_windows/permission_handler_windows_plugin.hheader is correct for Windows platform support.
22-23: Plugin registration added for Windows permission handler.The
PermissionHandlerWindowsPluginRegisterWithRegistrarcall follows the correct pattern for plugin registration and integrates the permission handler into the Windows Flutter plugin registry.makefile (1)
2-2: LGTM! Improved naming consistency for localization target.The renaming of
gen_localetolocalemakes the target name more concise while maintaining clarity. The changes are consistent across the .PHONY declaration, target reference, and target definition.Also applies to: 16-16, 19-19
lib/features/debug_tools/page/popups/debug_tools_list_dialog.dart (2)
4-4: LGTM! Updated import for the refactored dialog system.The import change aligns with the dialog system refactoring from
CustomDialogWrappertoCustomDialog.
22-48: LGTM! Clean migration to the new dialog API.The refactoring from
showDialogwithCustomDialogWrappertoCustomDialog.custom(...).show()maintains the same functionality while using the updated API. The context handling is correct, and the dialog content structure remains unchanged.lib/features/debug_tools/page/actions/debug_tools_actions_page_content.dart (1)
5-5: LGTM! Appropriate imports for notification permission functionality.The new imports support the notification permission request feature being added to the debug tools.
Also applies to: 8-8, 10-10
assets/localization/app_en.arb (1)
2-2: Well-organized localization structure.The reorganization with section markers (@@x---SECTION_NAME) provides excellent organization and makes the localization file more maintainable. The new PERMISSIONS_HANDLING section is appropriately placed and named.
Also applies to: 11-11, 21-21, 27-27, 33-33, 38-38, 42-42
HE-LU
force-pushed
the
helu/feat/notification_permission_handling
branch
from
9b94191 to
a1873c9
Compare
There was a problem hiding this comment.
Pull Request Overview
This pull request implements Notification permission handling and related UI updates. Key changes include:
- Adding the permission_handler dependency and Windows plugin support.
- Refactoring notification permission logic by migrating from Firebase Messaging to the permission_handler package.
- Updating dialog components and localization keys for consistent messaging, as well as making adjustments in iOS, Android, and build configuration.
Reviewed Changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| windows/flutter/generated_plugins.cmake | Added permission_handler_windows to the plugin list. |
| windows/flutter/generated_plugin_registrant.cc | Registered the Windows permission handler plugin. |
| pubspec.yaml | Added the permission_handler dependency. |
| project_setup/README.md | Corrected spelling in the rebranding section. |
| makefile | Renamed targets (e.g. from gen_locale to locale) for consistency. |
| lib/features/debug_tools/**/*.dart | Updated dialog usage and refined action logic for notification permission handling. |
| lib/common/provider/notifications_service.dart | Introduced permission checking and requesting functions using permission_handler. |
| ios/Runner/Info.plist & Podfile | Added necessary configuration for remote notifications support. |
| flutter.code-workspace | Added a marker snippet that appears unusual. |
| assets/localization/app_en.arb | Updated localization keys and messages for notifications and other features. |
| android/app/src/main/AndroidManifest.xml | Added POST_NOTIFICATIONS permission. |
| README.md | Updated documentation command from make build_runner to make watch. |
Comments suppressed due to low confidence (1)
makefile:16
- Renaming the 'gen_locale' target to 'locale' improves clarity; please ensure that corresponding documentation and references have been updated accordingly.
@make locale
Added implementation and sample for Notification Permission handling logic.