feat(reminders): database overhaul by ericli3690 · Pull Request #20325 · ankidroid/Anki-Android

ericli3690 · 2026-02-16T03:17:56Z

A bit of a big PR, but in my defense most of this is tests!

Error dialog (this is not going to be very user-facing, if all goes well no end users will ever need to see this dialog):

Purpose / Description

Implements a long-term solution for 2.24.0alpha3 is broken: Field 'onlyNotifyIfNoReviews' is required for type with serial name 'com.ichi2.anki.reviewreminders.ReviewReminder', but it was missing #20163 by adding a review reminder schema migration from v1 to v2.
Implements fallbacks so that even if a migration fails or review reminders become corrupted, the app doesn't become 100% blocked.
Abstracts the HashMap<ReviewReminderId, ReviewReminder> type in ReviewRemindersDatabase into a new interface, ReviewReminderGroup.
Fixes a bug where opening the app during the firing window of a review reminder when it hadn't fired yet would skip a notification. Makes it so that when the app is launched, any review reminder notifications that have been skipped for some reason (ex. the device was off) are immediately fired. To do this, the review reminder schema was migrated again from v2 to v3.
Cleans up and adds a lot of unit tests.

Fixes

Approach

Major reorganization of the ReviewReminderDatabase file. Creating a ReviewReminderGroup class was Arthur's idea!
For immediately firing missed notifications, the method employed is the standard one after discussing with Mike (i.e. store the timestamp every time a reminder's notifications attempt to fire, if no scheduled firing is detected while the app is scheduling reminders ex. on boot up, then immediately fire). We're not going to provide this as a configurable option like some other notification apps do because instantly firing missed notifications is a good default.

How Has This Been Tested?

Incrementally installing:

Before the onlyNotifyIfNoReviews change, then these changes, and
After the onlyNotifyIfNoReviews change, then these changes
all works.
Intentionally corrupting the reminders does not block the app, the offending review reminders are just deleted.
Tested on a physical Samsung S23, API 34.

Learning

Ran into a bunch of headaches around testing, Kotlin's deserialization library and type safety. Went through three separate ideas for solving the "opening the app causes a notification to be dropped" bug, got 90% of the way to the optimal solution independently, then asked Mike and realized I should have asked Mike earlier haha

Checklist

You have a descriptive commit message with a short title (first line, max 50 chars).
You have commented your code, particularly in hard-to-understand areas
You have performed a self-review of your own code
UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
UI Changes: You have tested your change using the Google Accessibility Scanner

Copilot

Pull request overview

This PR implements a comprehensive database overhaul for the review reminders feature, addressing critical bugs related to schema migration and adding support for detecting missed notifications. The changes fix issue #20163 where users upgrading from earlier versions encountered deserialization errors due to the missing onlyNotifyIfNoReviews field, and issue #20170 by adding extensive regression tests.

Changes:

Implements schema migration chain (V1 → V2 → V3) with proper fallback handling for corrupted data
Adds ReviewReminderGroup abstraction to replace verbose HashMap usage throughout the codebase
Introduces latestNotifTime tracking and shouldImmediatelyFire() logic to detect and immediately fire missed notifications when the app is launched
Significantly expands test coverage with 5 new/updated test files covering migration paths, immediate firing logic, and error handling

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
libanki/testutils/src/main/java/com/ichi2/anki/libanki/testutils/AnkiTest.kt	Adds test helper methods `withNotes()` and `withNote()` for convenient deck+note creation
AnkiDroid/src/test/java/com/ichi2/testutils/ext/ReviewRemindersDatabase.kt	New test extension providing `storeReminders()` helper for test setup
AnkiDroid/src/test/java/com/ichi2/anki/services/NotificationServiceTest.kt	Refactored tests with new verification helpers and latestNotifTime validation
AnkiDroid/src/test/java/com/ichi2/anki/services/AlarmManagerServiceTest.kt	Adds comprehensive tests for immediate notification firing logic
AnkiDroid/src/test/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabaseTest.kt	Extensively refactored tests for corruption handling, migration verification, and schema canary tests
AnkiDroid/src/test/java/com/ichi2/anki/reviewreminders/ReviewReminderTest.kt	Adds tests for `latestNotifTime` and `shouldImmediatelyFire()` logic
AnkiDroid/src/main/res/values/preferences.xml	Adds preference key for storing deserialization errors
AnkiDroid/src/main/java/com/ichi2/anki/settings/Prefs.kt	Adds `reviewReminderDeserializationErrors` preference property
AnkiDroid/src/main/java/com/ichi2/anki/services/NotificationService.kt	Updates latestNotifTime when recurring notifications fire
AnkiDroid/src/main/java/com/ichi2/anki/services/AlarmManagerService.kt	Implements immediate firing logic for missed notifications
AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ScheduleReminders.kt	Updates to use ReviewReminderGroup abstraction and improved error handling
AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabase.kt	Core database changes including migration logic, error handling, and scope validation
AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewReminderSchema.kt	New file containing schema version definitions and migration implementations
AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewReminderGroup.kt	New abstraction class wrapping HashMap for cleaner API
AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewReminder.kt	Adds latestNotifTime field and shouldImmediatelyFire() method
AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt	Checks for and displays deserialization errors on app start

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

criticalAY

Most of the code is clean, will review once more then will proceed forward

david-allison

This is AWESOME, thanks!

david-allison · 2026-03-03T18:43:43Z

+    /**
+     * Constructs a [ReviewReminderGroup] from a serialized JSON string.
+     *
+     * @throws SerializationException If the stored string is not a valid JSON string.
+     * @throws IllegalArgumentException If the decoded reminders map is not a HashMap<[ReviewReminderId], [ReviewReminder]>.
+     */
+    constructor(serializedString: String) : this(
+        Json.decodeFromString<HashMap<ReviewReminderId, ReviewReminder>>(serializedString),
+    )
+
+    /**
+     * Serializes this [ReviewReminderGroup] to a JSON string for storage.
+     */
+    fun serializeToString(): String = Json.encodeToString(underlyingMap)


Doesn't this have the same problem that if the version changes, then the serialization fails?

Maybe define this as a factory method instead of a constructor

Nope! In fact, this is the codepath used by the Database for parsing ReviewReminderGroup strings. When it throws serialization errors, those get caught by the Database's handling for it.

I considered using a factory method, but I think a constructor's kinda cute, let me know if you think otherwise.

...plus better unit tests Review Reminders Reverted the quick-and-dirty schema migration crash fix for onlyNotifyIfNoReviews and added a proper schema migration. Created a schema migration using the established system for review reminder schema migrations. Moved all subclasses of ReviewReminderSchema to ReviewRemindersDatabase so that ReviewReminderSchema can be made sealed. Improved unit tests and added regression tests to ensure that all future changes to ReviewReminder are caught.

Previously, ReviewRemindersDatabase used "HashMap<ReviewReminderId, ReviewReminder>" a lot. Arthur suggested making a small abstraction over this to 1. get rid of the verbosity that comes with saying "HashMap<ReviewReminderId, ReviewReminder>" over and over, and 2. limiting the functionality provided by "HashMap<ReviewReminderId, ReviewReminder>" to a more clearly-defined, clean, limited interface. I've implemented that here. The trickle-down effects of this abstraction cause multiple files, ex. tests, to change. I've also moved all the ReviewReminderSchema including old ReviewReminder schemas into a new, separate file to make ReviewRemindersDatabase shorter and cleaner. Previously, you could use ReviewRemindersDatabase.editRemindersForDeck and ReviewRemindersDatabase.editAllAppWideReminders to write deck-specific reminders to the app-wide key or vice versa. None of the code currently does so, but it really should not be allowed at all. I've added a `require` clause so that it causes a runtime exception if attempted, with unit tests. A better solution would probably be to create two separate kinds of ReviewReminder, one of which is DeckSpecific and the other of which is AppWide, so that trying to write one kind to the wrong key causes a compile-time error, but that would require a massive refactor of ReviewReminder and the schema migration system and, in my opinion, be overly complicated.

A new SharedPreference. Review reminders are deserialized and have their alarms scheduled when the device starts, but if the deserialization process fails and no valid migrations are available, the error can be put into this string so that the next time the user opens the app, an error dialog can be shown to inform them of the issue.

…ully Previously, if for some reason the review reminder system failed to deserialize or migrate review reminders, it would block the entire app from working due to trying to set review reminder alarms on launch. Obviously, a small side feature like review reminders should not block the entire app's function. This change substantially cleans up ReviewRemindersDatabase. If a deserialization error happens and no migration works, the offending ReviewReminderGroup is deleted, an error message is stored to SharedPreferences, and a crash report is sent. Then, the next time the app is opened and DeckPicker is opened, all accumulated error messages up to that point are shown and then cleared from SharedPreferences. We need to store the error messages and show them later on because it's possible for a review reminder database access to happen on device boot, not just when the app is open. If a deserialization error happens on ScheduleReminders, a wrapper function immediately shows the error message. Updates and streamlines tests for deserialization errors.

A future commit will use `upsertReminder` outside of ScheduleReminders, so they've been moved to ReviewRemindersDatabase.

Review Reminders We currently schedule all stored review reminders for their next upcoming alarm time when the application process starts up, such as when the app initially launches (we also have a BOOT_COMPLETED receiver). However, a notification "firing" is not just a single moment in time; since we don't have permissions to use proper alarm-clock-style alarms, the best we can do is a 10 minute `setWindow`, which means "firing" a notification is a non-deterministic event that happens some time in the ten minutes after a review reminder's scheduled notification time. Consider the following set of actions: 1. The user creates a review reminder for 1:00 pm, its first notification is scheduled for 1:00 pm on Monday 2. At 1:00, we enter the `setWindow` period 3. At 1:03, the notification still has not fired; curious as to why, the user opens up their app 4. This causes `scheduleAllNotifications` to trigger, which reschedules the review reminder notification for the next upcoming time, which is 1:00 pm on Tuesday 5. ... We've skipped a notification! To solution for this is to copy most notification systems: we record when the latest notification time was ourselves (it's impossible to read AlarmManager's currently-scheduled alarms programmatically) and check whenever scheduling a notification if the review reminder's notification has fired since its most recent scheduled time. If not, we immediately fire a notification. This also helps with missed notifications in general: if, for example, the user has had their device off for 6 hours and then turns it on, all alarms that fired during that interval should immediately show. This commit also creates unit tests surrounding this feature and enhances the readability of NotificationServiceTest. The ReviewReminderSchema is migrated from v2 to v3.

david-allison

Looks incredible! Thanks

david-allison · 2026-04-12T10:41:08Z

+                    // Log error, it will be displayed to the user either immediately if the app is open or when they next open the app if not
+                    val errorString = "Encountered (${e.message}) while parsing $jsonString"
+                    Prefs.reviewReminderDeserializationErrors = Prefs.reviewReminderDeserializationErrors.orEmpty() + "[$errorString]"
+                    Timber.e(e, errorString)


I think I'd like to keep this as a .e, at least if I understand how Timber.e works for AnkiDroid. This line should literally never run. If it does, then something's gone very wrong.

criticalAY

Time to get this in, thanks for your efforts Eric!!