feat(in-app-notification): Create NotificationSender to manage notification providers

[rafaeltonholo]

Resolves #9245

This pull request introduces a NotificationSender responsible for managing and dispatching notifications through various providers. As the application now supports both in-app and system notifications, this manager simplifies the process of triggering a notification by abstracting the provider-specific logic.

The NotificationSender receives a notification object and determines the appropriate provider to use for delivery.

Changes Included

• NotificationSender Implementation: Core manager for handling notification logic.
• InAppNotificationNotifier: Implementation for displaying notifications within the app UI.
• SystemNotificationNotifier: Implementation for triggering native system notifications.
• Enhanced Notification Features:
  • Added support for notification actions, allowing for interactive notifications.
  • Included notification icons and custom styling capabilities.
• Debug Tools: A new secret debug settings screen has been added to facilitate testing of the notification system and other options in the future.
• UI Components:
  • Added a horizontal divider atom to the design system.
  • Resolved rendering issues with theme previews.

[wmontwe]

Thank you for your work on the notification system overhaul. I've partially reviewed the code and have several observations that I'd like to share to help improve both this PR and future contributions.

I'm wondering if the debug section should work as it doesn't trigger any notifications on my side.

Complexity and API Surface

The notification system you've designed is quite comprehensive, but the API surface is unnecessarily large and complex. For example:

• The hierarchy of notification types (SystemNotification, InAppNotification, various subtypes like MailNotification.NewMail.SingleMail, PushServiceNotification, etc.) creates a complex inheritance tree
• The extensive use of sealed interfaces and classes adds layers of abstraction that make the code harder to follow
• The notification styling system with multiple styles (NotificationStyle.System.BigTextStyle, NotificationStyle.System.InboxStyle, etc.) adds another dimension of complexity

Consider simplifying the API by:

• Reducing the number of notification types and consolidating similar ones
• Flattening the inheritance hierarchy where possible
• Using composition over inheritance for specialized behavior

Test Coverage

Despite the complexity of the implementation, the tests are sparse. This makes it difficult to verify the behavior of the code changes and increases the risk of regressions. For a system as critical as notifications, comprehensive test coverage is essential.

I would recommend:

• Adding unit tests for each notification type
• Creating integration tests that verify the end-to-end notification flow
• Adding tests for edge cases (e.g., permission denied, notification channels not available)
• Including tests for the command pattern implementation

PR Size and Structure

The PR is too large and tackles too many concerns at once. This makes it difficult to review thoroughly and increases the chance of issues slipping through. Breaking it down into smaller, focused PRs would have been more manageable.

Specific components that could have been separate PRs:

1. Debug Tools: The feature/debug-settings module with its comprehensive UI for testing notifications could have been a standalone PR
2. Parcelize Support: The KMP parcelize implementation in core/common could have been separated
3. Notification API: The base interfaces and types could have been introduced first
4. System Notification Implementation: Android-specific implementation
5. In-App Notification Implementation: The broadcast mechanism
6. Resources: The notification icons
7. Legacy Integration: Connection with existing notification settings

Architectural Concerns

The command pattern implementation (NotificationCommandFactory, SystemNotificationCommand, etc.) is a good approach for handling different notification types, but it adds complexity without clear documentation on how it fits into the larger architecture.

Recommendations

For this PR:

1. Consider simplifying the API before merging
2. Add more tests to cover critical functionality
3. Provide better documentation explaining the architectural decisions

For future PRs:

1. Break down large features into smaller, focused PRs
2. Implement core functionality before adding specialized features
3. Add tests alongside implementation
4. Consider using feature flags to merge incomplete features safely

I appreciate the effort put into this comprehensive notification system, but addressing these concerns would make it more maintainable and easier to integrate into the codebase.

[wmontwe]

This should be alias(libs.plugins.kotlin.parcelize)

[wmontwe]

What about adding the icons here directly instead of the design system? Otherwise we could consider moving icons to their own module.

[wmontwe]

It this required?

[rafaeltonholo]

Hi @wmontwe, thanks for reviewing the PR! I will answer your comments/concerns in separate comments, so it makes it easier to discuss each section

[rafaeltonholo]

PR Size and Structure

I agree with you, the PR is quite long. Would you prefer me splitting it into different PRs right now so it would be easier to focus on some areas?

[rafaeltonholo]

Complexity and API Surface

About the hierarchy:

The concept of using sealed classes with a nested hierarchy for notifications is designed to make it easier for developers to locate a specific notification based on its group or name.

For instance, if someone wants to trigger a notification after a new email is sent but encounters a failure, they could search for:

• MailNotification
• SendFail

By searching for MailNotification, they would also find the relevant nested notification, SendFail, making it straightforward to send the appropriate notification.

I find this approach to be organized rather than complex. However, if you believe that flattening the notifications and creating a Notification class without utilizing sealed classes would enhance the API surface, I can certainly do that.

About the Notification styling system:

I need to highlight that there is a difference between a Notification type (AuthenticationErrorNotification) and a Notification style (NotificationStyle.System.BigTextStyle).

The same notification can be displayed via different providers (System and/or In-App) and with different styles. Additionally, in the case of an in-app notification, we could show the same notification using multiple styles.

The changes present in this PR only consider the system notification styles we are already using in the app. Nonetheless, this new API is making it a bit more platform-agnostic. Even though we use the same names as the Android Notification system, we could implement the visual representation of it differently depending on the platform.

Regarding the number of notification types and consolidating similar ones

The current types of notifications are a direct one-to-one mapping of each notification that we are already triggering in the app.

Apart from the SingleMail and SummaryMail, which I believe could be merged into a single notification, I don’t see many other types that could be combined. This is unless we decide to eliminate some notifications that we are currently sending to users.

I’m open to any suggestions you might have.

Using composition over inheritance for specialized behaviour

To be honest, I'm not sure where that would be applied here. Could you please point out where you believe that could be applied?

[rafaeltonholo]

Test Coverage

Adding unit tests for each notification type

I will add them.

Creating integration tests that verify the end-to-end notification flow

Sure, I'll try to add them.

Adding tests for edge cases (e.g., permission denied, notification channels not available)

These are all related to the System Notification command execution and is going to be handled in a different task (#9391)

Including tests for the command pattern implementation

I will add them.

[rafaeltonholo]

Architectural Concerns

adds complexity without clear documentation on how it fits into the larger architecture.

There is a task (#9252) that focuses on documenting the new API. Is that what you are referring to?

Additionally, I would appreciate it if you could provide a few specific examples of what you would like me to emphasize in the documentation. Thank you!

[rafaeltonholo]

This PR will be split into smaller PRs to improve the review