fix(android): give every notifier a distinct notification id (#297) - #304
Merged
Conversation
notify() is tagless, so notifiers sharing an id silently replace each other's undismissed notifications. On main, id 4203 was posted by InactivityNotifier, BatteryAlertNotifier.NOTIF_ID_LOW and ScheduledReportNotifier.MORNING; id 4204 by SmartAlarmNotifier, BatteryAlertNotifier.NOTIF_ID_FULL and .WORKOUT — so e.g. a low-battery warning wiped the morning recap. Reassign to a clean, fully-distinct map: 4201 connection, 4202 illness, 4203 inactivity, 4204 smart alarm, 4205/4206/4207 battery (runtime/low/full), 4208/4209 scheduled report (morning/workout). (Corrects #289, which moved NOTIF_ID_LOW to 4205 — already taken by the battery NOTIF_ID_RUNTIME in the same file — and bundled an unrelated AUDIT.md. Battery needs THREE ids, not two.)
This was referenced Jul 12, 2026
ryanbr
added a commit
that referenced
this pull request
Jul 18, 2026
…he calendar day (#567) (#589) The morning recap re-fired at midnight for anyone up late. ScheduledReportNotifier.onMorning deduped on LocalDate.now() (the phone's CALENDAR day, rolls at midnight), but the recap reports a banked NIGHT and _today resolves via the logical day (rolls at 04:00, #304 carve-out). For a late-nighter crossing midnight without sleeping, the new calendar day has no banked night, so resolveTodayRow falls back to LAST night's row (totalSleepMin != null) — while the dedup key had already advanced to the new calendar day, so `lastNotifiedDay != today` went true again and the recap re-fired with last night's data ("sent at the start of a new day"). Fix: dedup on the REPORTED NIGHT's day (the resolved today-row's `day`), not the calendar day. - onMorning gains a reportDay param; AppViewModel passes todayRow.day. - Policy param renamed today -> reportDay with a doc note (prevents a regression back to LocalDate.now()). Logic unchanged: enabled && present && lastNotifiedDay != reportDay. At midnight the row still resolves to last night (reportDay unchanged) → already notified → no re-fire. The real morning's new-night row fires normally. No pref migration: reportMorningDay stays an ISO yyyy-MM-dd string; a calendar day and a row day compare the same, so an upgrading user simply fires normally the next morning. Android-only (the morning recap is an Android feature; no iOS twin). Strap-agnostic (day-key logic). Tested: ./gradlew compileFullDebugKotlin + testFullDebugUnitTest --tests ScheduledReportPolicyTest -> 16/16 (added a midnight-rollover regression: reportDay = last night's day → suppressed).
DX23876
pushed a commit
to DX23876/noop
that referenced
this pull request
Jul 22, 2026
) (ryanbr#304) notify() is tagless, so notifiers sharing an id silently replace each other's undismissed notifications. On main, id 4203 was posted by InactivityNotifier, BatteryAlertNotifier.NOTIF_ID_LOW and ScheduledReportNotifier.MORNING; id 4204 by SmartAlarmNotifier, BatteryAlertNotifier.NOTIF_ID_FULL and .WORKOUT — so e.g. a low-battery warning wiped the morning recap. Reassign to a clean, fully-distinct map: 4201 connection, 4202 illness, 4203 inactivity, 4204 smart alarm, 4205/4206/4207 battery (runtime/low/full), 4208/4209 scheduled report (morning/workout). (Corrects ryanbr#289, which moved NOTIF_ID_LOW to 4205 — already taken by the battery NOTIF_ID_RUNTIME in the same file — and bundled an unrelated AUDIT.md. Battery needs THREE ids, not two.)
DX23876
pushed a commit
to DX23876/noop
that referenced
this pull request
Jul 22, 2026
…he calendar day (ryanbr#567) (ryanbr#589) The morning recap re-fired at midnight for anyone up late. ScheduledReportNotifier.onMorning deduped on LocalDate.now() (the phone's CALENDAR day, rolls at midnight), but the recap reports a banked NIGHT and _today resolves via the logical day (rolls at 04:00, ryanbr#304 carve-out). For a late-nighter crossing midnight without sleeping, the new calendar day has no banked night, so resolveTodayRow falls back to LAST night's row (totalSleepMin != null) — while the dedup key had already advanced to the new calendar day, so `lastNotifiedDay != today` went true again and the recap re-fired with last night's data ("sent at the start of a new day"). Fix: dedup on the REPORTED NIGHT's day (the resolved today-row's `day`), not the calendar day. - onMorning gains a reportDay param; AppViewModel passes todayRow.day. - Policy param renamed today -> reportDay with a doc note (prevents a regression back to LocalDate.now()). Logic unchanged: enabled && present && lastNotifiedDay != reportDay. At midnight the row still resolves to last night (reportDay unchanged) → already notified → no re-fire. The real morning's new-night row fires normally. No pref migration: reportMorningDay stays an ISO yyyy-MM-dd string; a calendar day and a row day compare the same, so an upgrading user simply fires normally the next morning. Android-only (the morning recap is an Android feature; no iOS twin). Strap-agnostic (day-key logic). Tested: ./gradlew compileFullDebugKotlin + testFullDebugUnitTest --tests ScheduledReportPolicyTest -> 16/16 (added a midnight-rollover regression: reportDay = last night's day → suppressed).
DX23876
added a commit
to DX23876/noop
that referenced
this pull request
Aug 1, 2026
…dditively Three correctness follow-ups to the reviewed revision. Day keying. applyingEditedSleepSessions grouped its sessions with a single current-zone offset applied across all history, while userEditedDays — which decides where the debt correction lands — resolves the offset at each session's own end instant. The two could therefore disagree about which day a night belongs to across a DST boundary, leaving the rebuilt sleep fields on one day and the debt correction on another. The keyer existed in three copies, one of them wrong; it is now Repository.sleepEndDayKey and userEditedDays, mergeSleep and the edited-day overlay all go through it. This is the ryanbr#406 (Swift) / ryanbr#304 (Android) local wake-day rule that MergeSleepLocalDayTest pins. Owner probe order. sleepOwnerIds was computedReadIds + importedReadIds, putting the CANONICAL computed namespace ahead of the ACTIVE imported one. That demotes the active imported source relative to the previous computed-then-imported chain and lets a coincidental same-detectedStartTs row capture an edit — the failure the single fallback was written to avoid. The order is now strictly additive over the pre-union behaviour: the active strap's two namespaces keep their exact previous order, and the canonical pair is appended as a fallback that was not probed at all before, so no case that worked before can regress. The doc comment also claimed computed rows outrank imported ones, which SleepMerge.merge documents the other way round; corrected. Android edited-day set. It tried a UTC key first and fell back to UTC, which re-opens ryanbr#304 for a UTC+ user who wakes after local midnight but before UTC midnight. It now uses localDayString, the helper this same file already uses eight lines above to group sleeps by local day. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
12 tasks
DX23876
added a commit
to DX23876/noop
that referenced
this pull request
Aug 1, 2026
…dditively Three correctness follow-ups to the reviewed revision. Day keying. applyingEditedSleepSessions grouped its sessions with a single current-zone offset applied across all history, while userEditedDays — which decides where the debt correction lands — resolves the offset at each session's own end instant. The two could therefore disagree about which day a night belongs to across a DST boundary, leaving the rebuilt sleep fields on one day and the debt correction on another. The keyer existed in three copies, one of them wrong; it is now Repository.sleepEndDayKey and userEditedDays, mergeSleep and the edited-day overlay all go through it. This is the ryanbr#406 (Swift) / ryanbr#304 (Android) local wake-day rule that MergeSleepLocalDayTest pins. Owner probe order. sleepOwnerIds was computedReadIds + importedReadIds, putting the CANONICAL computed namespace ahead of the ACTIVE imported one. That demotes the active imported source relative to the previous computed-then-imported chain and lets a coincidental same-detectedStartTs row capture an edit — the failure the single fallback was written to avoid. The order is now strictly additive over the pre-union behaviour: the active strap's two namespaces keep their exact previous order, and the canonical pair is appended as a fallback that was not probed at all before, so no case that worked before can regress. The doc comment also claimed computed rows outrank imported ones, which SleepMerge.merge documents the other way round; corrected. Android edited-day set. It tried a UTC key first and fell back to UTC, which re-opens ryanbr#304 for a UTC+ user who wakes after local midnight but before UTC midnight. It now uses localDayString, the helper this same file already uses eight lines above to group sleeps by local day.
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
1 participant
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 #297. Reimplements #289 with the collision corrected.
The bug
notify()is called tagless, so notifiers sharing an id silently replace each other's undismissed notifications. On main:NOTIF_ID_LOW, ScheduledReportNotifier.MORNING(3-way)NOTIF_ID_FULL, ScheduledReportNotifier.WORKOUT(3-way)A low-battery warning could wipe the morning recap, a move reminder wipe the low-battery warning, etc.
Fix
Fully-distinct map:
4201connection ·4202illness ·4203inactivity ·4204smart alarm ·4205/4206/4207battery (runtime/low/full) ·4208/4209scheduled report (morning/workout). Verified all nine ids are unique.Why not #289
#289 moved
NOTIF_ID_LOWto 4205 — butNOTIF_ID_RUNTIMEin the same file is already 4205, so it traded one collision for another (battery has three posters, not two). It also bundled an unrelated 200-lineAUDIT.md. This drops both problems.Android-only;
compileFullDebugKotlinclean; ids verified distinct by grep.