Repository navigation
Notification System Refactor - #1923
Draft
AugustoVSoares wants to merge 1 commit into
Draft
AugustoVSoares wants to merge 1 commit into
AugustoVSoares wants to merge 1 commit into
Conversation
… and added a debu test notification (must be remove when branch is merged)
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
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. Comment |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1772
Refactored the notification system so that adding a new notification type is a
declaration rather than an implementation.
Before:
Notificationwas an abstract class holdinguniqueID+timeout,plus three methods each subclass had to write:
shouldDisplay(),buildNotificationContent()anddisplayNotification(). Every type hand-rolled its ownAndroidNotificationDetails/DarwinNotificationDetailspair and its own throttle logic, so the platform presentation config was duplicated per type and the Workmanager isolate init was easy to get wrong.After:
Notificationis an abstract interface of getters. Channel/image/group config, id resolution, the settings toggle check and the throttle are all implemented once on the base class and inNotificationManager. A new type declares its identity, its channel, its id strategy and its trigger, and nothing else.Debugging
There's currently a debugging test notification class that can be triggered and toggled to show up every 30 seconds in the user profile. It is gated behind
kDebugMode, but it is committed in this PR and is marked with// WARN: Remove this before merging to develop. It should be stripped in a follow-up commit so the framework change stays reviewable on its own.Note: it is deliberately not in
notificationMap, that map is the production registry consumed by the Android Workmanager task and the iOS background task, andTestNotification._shouldFirereturnstrueunconditionally withdefaultEnabled: trueand no user-facing switch. Registering it there would ship a test notification to every user. The tradeoff is that it does not exercise the real background-isolate path, only the in-app poll.Planned features TBA in this PR:
ScheduledTriggertypeUI changes
DEBUG: test notifications


DEBUG: notification appearance
Other important mentions
iOS support still needs to be tested.
ScheduledTriggerhas no consumers yet.PollTrigger. The scheduling path is implemented but unexercised; it will also needSCHEDULE_EXACT_ALARMon Android 12+ and rescheduling on boot before it's used in production.Review checklist
View Changes
Performance