Skip to content

Conversation

lollipopkit
Copy link
Owner

@lollipopkit lollipopkit commented Sep 14, 2025

Fixes #912 #909

Summary by Sourcery

Enforce POST_NOTIFICATIONS permission checks on Android 13+ before creating channels or starting the foreground service, improve error handling and logging in ForegroundService, and enhance MainActivity to request and handle notification permissions and secure broadcast receiver registration.

Bug Fixes:

  • Gracefully stop ForegroundService when notification permission is denied or intent is null
  • Prevent security exceptions when starting or updating the foreground service without notification permission
  • Handle exceptions when unregistering MainActivity broadcast receiver to avoid crashes

Enhancements:

  • Add detailed logging and try/catch around notification channel creation, service start/stop, and receiver operations
  • Use ServiceInfo.FOREGROUND_SERVICE_TYPE_DATA_SYNC flag for startForeground on Android Q+
  • Show permission rationale and log request results in MainActivity on Android 13+
  • Register broadcast receiver with RECEIVER_NOT_EXPORTED flag on Android 13+ for security

Copy link

sourcery-ai bot commented Sep 14, 2025

Reviewer's Guide

Implements Android 13+ notification permission checks, enhances error handling and logging around notification channel creation and the foreground service lifecycle, updates notification action building, and improves MainActivity’s permission request and receiver registration flow.

Sequence diagram for notification permission check and foreground service start (Android 13+)

sequenceDiagram
    participant MainActivity
    participant ForegroundService
    participant NotificationManager
    actor User

    User->>MainActivity: Launch app
    MainActivity->>MainActivity: reqPerm()
    MainActivity->>User: Request POST_NOTIFICATIONS permission (Android 13+)
    User-->>MainActivity: Grant or deny permission
    MainActivity->>ForegroundService: Start service
    ForegroundService->>ForegroundService: Check notification permission
    alt Permission granted
        ForegroundService->>NotificationManager: Create notification channel
        ForegroundService->>ForegroundService: Start foreground service
    else Permission denied
        ForegroundService->>ForegroundService: Log warning
        ForegroundService->>ForegroundService: stopSelf()
    end
Loading

File-Level Changes

Change Details Files
Add notification permission checks and graceful termination in ForegroundService
  • Check POST_NOTIFICATIONS in onStartCommand and replace premature stopForegroundService with stopSelf
  • Double-check permission before starting/updating foreground in ensureForeground and stop service if missing
  • Catch SecurityException in ensureForeground to handle missing permissions gracefully
android/app/src/main/kotlin/tech/lolli/toolbox/ForegroundService.kt
Wrap notification channel creation in try-catch and log outcomes
  • Enclose getSystemService and createNotificationChannel calls in try-catch
  • Log success and error paths using Log.d and custom logError
android/app/src/main/kotlin/tech/lolli/toolbox/ForegroundService.kt
Use explicit foreground service type on Android Q+
  • Call startForeground with FOREGROUND_SERVICE_TYPE_DATA_SYNC for SDK >= Q
  • Fallback to standard startForeground on earlier releases
android/app/src/main/kotlin/tech/lolli/toolbox/ForegroundService.kt
Enhance notification action construction
  • Replace simple addAction with Notification.Action.Builder and Icon.createWithResource
  • Ensure actions are built rather than using deprecated overloads
android/app/src/main/kotlin/tech/lolli/toolbox/ForegroundService.kt
Refine stopForegroundService logic
  • Only call stopForeground(STOP_FOREGROUND_REMOVE) if isFgStarted is true
  • Reset isFgStarted flag after stopping
android/app/src/main/kotlin/tech/lolli/toolbox/ForegroundService.kt
Improve permission request flow in MainActivity
  • Wrap requestPermissions call in try-catch and log errors
  • Add rationale logging when user previously denied notifications
  • Implement onRequestPermissionsResult to log grant or denial
android/app/src/main/kotlin/tech/lolli/toolbox/MainActivity.kt
Update broadcast receiver registration/unregistration
  • Use ContextCompat.registerReceiver with RECEIVER_NOT_EXPORTED on SDK >= Tiramisu
  • Wrap unregisterReceiver in try-catch to avoid crashes
android/app/src/main/kotlin/tech/lolli/toolbox/MainActivity.kt

Assessment against linked issues

Issue Objective Addressed Explanation
#912 Fix the app crash or failure to start after updating via F-Droid on Android 14 (app version 1.0.1253).

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@lollipopkit lollipopkit linked an issue Sep 14, 2025 that may be closed by this pull request
Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey there - I've reviewed your changes and they look great!


Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@lollipopkit
Copy link
Owner Author

@claude review

Copy link

claude bot commented Sep 14, 2025

Claude finished @lollipopkit's task —— View job


Code Review Complete ✅

I've thoroughly reviewed the Android notification permission fixes. This is a well-implemented solution that addresses Android 13+ notification permission requirements with proper error handling.

Todo List:

  • Read and analyze ForegroundService.kt changes
  • Read and analyze MainActivity.kt changes
  • Review Android permission handling implementation
  • Check error handling and logging patterns
  • Provide comprehensive review feedback

Review Summary

Overall Assessment: Excellent ⭐⭐⭐⭐⭐

This PR correctly implements Android 13+ notification permission handling with robust error handling, logging, and graceful degradation. The code follows Android best practices and properly addresses the SecurityException issues mentioned in the linked issues.

Strengths ✅

  1. Proper Permission Checks: Correctly implements POST_NOTIFICATIONS permission checks for Android 13+ (API 33+)

    • ForegroundService.kt:54-63 and ForegroundService.kt:133-141
    • Uses proper API level checks with Build.VERSION_CODES.TIRAMISU
  2. Graceful Error Handling: Service stops gracefully when permission is denied instead of crashing

    • Uses stopSelf() instead of throwing exceptions
    • Prevents SecurityException when starting foreground service
  3. Comprehensive Logging: Excellent logging implementation with both system logs and file logging

    • Custom logError() method provides detailed error tracking
    • Debug logs for successful operations help with troubleshooting
  4. Secure Broadcast Receiver: Properly uses RECEIVER_NOT_EXPORTED flag for Android 13+

    • MainActivity.kt:172 follows security best practices
  5. Modern Android API Usage:

    • Uses ServiceInfo.FOREGROUND_SERVICE_TYPE_DATA_SYNC for Android Q+
    • Modern notification action building with Notification.Action.Builder

Technical Implementation Details ✅

ForegroundService Changes:

  • Lines 54-63: Early permission check in onStartCommand() prevents service start without permission
  • Lines 133-141: Double-check permission before calling startForeground()
  • Lines 159-162: SecurityException handling prevents crashes from permission-related exceptions
  • Lines 144-148: Proper foreground service type specification for Android Q+
  • Lines 227-233: Modern notification action building

MainActivity Changes:

  • Lines 105-127: Permission request with rationale checking
  • Lines 129-135: Helper method for permission status checking
  • Lines 42-46, 73-77: Prevent service operations when permission denied
  • Lines 171-175: Secure broadcast receiver registration
  • Lines 178-192: Permission result handling with logging

Minor Suggestions for Future Consideration 🔍

  1. Permission Rationale UX: Consider showing a dialog explaining why notification permission is needed when shouldShowRequestPermissionRationale() returns true

  2. Error Reporting to Flutter: The permission denial errors are properly returned to Flutter layer, which is good for user experience

  3. Log File Management: The log file writing is good, but consider log rotation or size limits for production apps

Security Assessment ✅

  • No security vulnerabilities identified
  • Proper use of RECEIVER_NOT_EXPORTED flag
  • Appropriate permission checks before sensitive operations
  • No hardcoded secrets or sensitive data exposure

Code Quality ✅

  • Clean, readable Kotlin code
  • Consistent error handling patterns
  • Proper resource cleanup in onDestroy()
  • Good separation of concerns between service and activity

Conclusion

This PR successfully addresses the Android 13+ notification permission issues (fixes #912 #909) with a robust, well-tested implementation. The code follows Android development best practices and handles edge cases gracefully. The implementation should prevent the crashes and service start failures mentioned in the issues.

Recommendation: ✅ APPROVE - Ready to merge


@lollipopkit lollipopkit merged commit 275581c into main Sep 14, 2025
3 checks passed
@lollipopkit lollipopkit deleted the lollipopkit/issue912 branch September 14, 2025 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to run the app after last update 1253版本闪退
1 participant