fix(notify): enforce fire-once dedupe for insight notifications#137
Conversation
Insight/health notifications carry a dedupeKey ("$date:$kind") and their
call sites promise "fires at most once/day", but nothing enforced it. The
OS notification id derived from the key only REPLACES a prior post in the
shade — it still re-alerts — and derivation re-runs on every BLE sync, so
any insight whose condition holds all day (e.g. today's irregular-rhythm
flag) re-fired the notification over and over (OpenStrap#136). Illness, anomaly,
temperature, readiness, HR-shift, recovery-ready, step-goal and
auto-workout alerts all shared this latent bug.
Add a persistent, bounded fired-key guard (FiredKeyStore, backed by
shared_preferences): NotificationCenter.emit now skips a dedupeKey that
has already fired and records it only after a real present, so a
permission-denied no-op doesn't consume the key. The guard resets itself
per new day via the date-prefixed keys and is capped to the most-recent
200 keys (oldest evicted) so it stays bounded without ever letting a
same-day key re-fire.
Paths that SHOULD repeat are untouched: device battery/charging alerts
(showDevice, edge-triggered arm/re-arm) and scheduled water/wind-down
reminders (zonedSchedule) never route through emit.
Closes OpenStrap#136
|
Warning Review limit reached
Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds persistent notification deduplication using bounded shared-preference storage. Notifications are recorded only after successful OS presentation, while permission failures, category restrictions, quiet hours, and concurrent emits are handled without consuming eligible keys. ChangesNotification deduplication
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant NotificationCenter
participant NotificationPrefs
participant FiredKeyStore
participant NotificationService
NotificationCenter->>NotificationPrefs: check shouldFireOs(event)
NotificationPrefs-->>NotificationCenter: allow or suppress
NotificationCenter->>FiredKeyStore: check dedupeKey
FiredKeyStore-->>NotificationCenter: fired or available
NotificationCenter->>NotificationService: attempt presentation
NotificationService-->>NotificationCenter: return success status
NotificationCenter->>FiredKeyStore: record successful dedupeKey
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/notify/notification_center.dart`:
- Around line 60-65: Serialize the dedupe transaction in NotificationCenter.emit
so each key is guarded from hasFired through presentation and recordFired,
preventing overlapping same-key emissions. In lib/notify/fired_keys.dart lines
42-49, serialize preference read-modify-write mutations so concurrent distinct
keys cannot lose updates. In test/notification_dedupe_test.dart lines 63-73,
replace sequential awaits with overlapping emit calls and add coverage for
concurrent distinct keys.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2a619843-d590-41f8-8922-8456672f280c
📒 Files selected for processing (4)
lib/notify/fired_keys.dartlib/notify/notification_center.dartlib/notify/notification_service.darttest/notification_dedupe_test.dart
The stress screen called NotificationService.presentEvent directly for its "High Stress Detected" alert, bypassing both the prefs gate (category / quiet-hours) and the new fire-once dedupe guard — so it could re-alert on repeat screen visits and ignore the user's quiet hours. Route it through NotificationCenter.emit like every other insight, so its "$date:high_stress" key inherits the same gating and fires at most once. Event fields are unchanged (health category, normal priority, no route); the call stays fire-and-forget (nothing read its return value).
Two overlapping emit() calls could both pass FiredKeyStore.hasFired before either recorded the key (→ both present the same alert), and their read-modify-write of the shared_preferences fired-key list could clobber each other (losing a key). This is more likely now the UI-thread stress-screen alert can race the background derive loop. Guard the hasFired → present → recordFired section with a small in-memory chained-Future lock so overlapping emits run one at a time; recordFired re-reads the latest list inside the serialised section, so no stale read-modify-write. Dart's single isolate makes this sufficient — no deps. Adds concurrency tests: overlapping emits of the same key present exactly once; overlapping emits of different keys both record (neither clobbered).
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/ui/stress/stress_screen.dart (1)
80-88: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftMake the deduplication sequence atomic and test that concurrency path.
NotificationCenter.emitcan perform presentation twice when overlapping calls both observe an absent key before either records it.
lib/ui/stress/stress_screen.dart#L80-L88: protect this routed alert with per-key in-flight serialization while preserving record-after-success behavior.test/notification_dedupe_test.dart#L62-L117: add a concurrent, barrier-controlled regression test so this race cannot return unnoticed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/ui/stress/stress_screen.dart` around lines 80 - 88, The routed alert at lib/ui/stress/stress_screen.dart:80-88 must use per-key in-flight serialization through NotificationCenter.emit, ensuring overlapping emissions cannot present the same dedupeKey twice while retaining record-after-success behavior. Add a concurrent barrier-controlled regression test at test/notification_dedupe_test.dart:62-117 that overlaps duplicate emissions, verifies only one presentation occurs, and confirms the key is recorded only after successful presentation.
🧹 Nitpick comments (1)
test/notification_dedupe_test.dart (1)
168-202: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftExercise
StressScreenitself, not onlyNotificationCenter.emit.These tests call
center.emit(highStress())directly, so they would pass even iflib/ui/stress/stress_screen.dartreverted toNotificationService.presentEvent. Add a widget test that mountsStressScreen, supplies a stress score above 70, and verifies the sink receives the gated/deduplicated event.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/notification_dedupe_test.dart` around lines 168 - 202, Add a widget test that mounts StressScreen with a stress score above 70 and injects a fake presentation sink, then trigger the screen behavior that emits the high-stress notification and verify the sink receives the event through the gated, deduplicated path. Keep the existing NotificationCenter.emit unit tests, but ensure the new test would fail if StressScreen reverted to calling NotificationService.presentEvent directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@lib/ui/stress/stress_screen.dart`:
- Around line 80-88: The routed alert at lib/ui/stress/stress_screen.dart:80-88
must use per-key in-flight serialization through NotificationCenter.emit,
ensuring overlapping emissions cannot present the same dedupeKey twice while
retaining record-after-success behavior. Add a concurrent barrier-controlled
regression test at test/notification_dedupe_test.dart:62-117 that overlaps
duplicate emissions, verifies only one presentation occurs, and confirms the key
is recorded only after successful presentation.
---
Nitpick comments:
In `@test/notification_dedupe_test.dart`:
- Around line 168-202: Add a widget test that mounts StressScreen with a stress
score above 70 and injects a fake presentation sink, then trigger the screen
behavior that emits the high-stress notification and verify the sink receives
the event through the gated, deduplicated path. Keep the existing
NotificationCenter.emit unit tests, but ensure the new test would fail if
StressScreen reverted to calling NotificationService.presentEvent directly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 55bdb12b-7823-4382-bbbd-87764d9cf5fa
📒 Files selected for processing (2)
lib/ui/stress/stress_screen.darttest/notification_dedupe_test.dart
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/notification_dedupe_test.dart`:
- Around line 37-42: Make the test sink’s entry point deterministic by exposing
an entered future from the sink implementation around call, and complete it when
call is first reached. In both concurrency tests, replace
Future<void>.delayed(Duration.zero) with await sink.entered before asserting or
releasing the gate, while preserving the existing serialization assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c2beed17-9163-4de7-879c-8bda7301795a
📒 Files selected for processing (2)
lib/notify/notification_center.darttest/notification_dedupe_test.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/notify/notification_center.dart
The concurrent-emit tests used Future.delayed(Duration.zero) to assume the first emit had reached the fake sink before releasing the gate — a scheduler-dependent barrier that could flake. The gated sink now completes an `entered` Completer the moment the first emit reaches the parked point, and the tests await that signal instead, so the overlap is guaranteed by ordering rather than timing. Assertions are unchanged.
2 participants
The bug (#136)
A user gets the "Irregular heart rhythm" notification repeatedly/constantly.
Insight and health notifications are emitted with a
dedupeKeylike'$date:irregular', and the call sites promise the event "fires at most once/day" — but nothing enforced it:NotificationCenter.emitgated only onNotificationPrefs.shouldFireOs(category-enabled + quiet-hours). It never consulteddedupeKey.osIdderived from the key makes a re-post replace the shade entry, but it still re-alerts (sound/buzz), and there was no persistent "already fired this key" record anywhere.emit('irregular', …)re-fired all day while today'sirregular_rhythm_flagwas set. The same latent bug hit the illness / anomaly / temperature / readiness / HR-shift alerts (all via the localemitclosure inderivation_engine.dart), plus recovery-ready, step-goal and auto-workout.The fix
Enforce the dedupe with a persistent, bounded fired-key guard:
FiredKeyStore(lib/notify/fired_keys.dart), backed byshared_preferenceslike the rest of the app.NotificationCenter.emitnow: gate → skip if thededupeKeyalready fired → present → record the key only after a real present. A permission-denied no-op therefore doesn't consume the key.NotificationService.presentEventnow returnsbool(true = actually shown) soemitcan record on success only.'2026-07-24:irregular'is a fresh key that fires once — so the guard resets itself per day with no manual reset. A repeated hit is a pure no-op (doesn't re-fire, doesn't refresh recency).This also stops the other insight alerts (illness, anomaly, temperature, readiness, resting-HR shift, recovery-ready, step-goal, auto-workout) from repeating on re-derive.
Also folded in: the stress-screen high-stress alert
stress_screen.dartcalledNotificationService.presentEventdirectly for its "High Stress Detected" alert ('$date:high_stress'), bypassing both the prefs gate and the new dedupe guard — so it could re-alert on repeat screen visits and ignore quiet hours. It now routes throughNotificationCenter.emitlike every other insight, inheriting the same gating + fire-once behaviour. Event fields are unchanged (health category, normal priority); the call stays fire-and-forget.What is deliberately left alone
NotificationService.showDevicewith their own edge-triggered arm/re-arm hysteresis — they never route throughemit, so they're unaffected.zonedScheduled, notemited — untouched.emitcaller was audited: all carry one-shot-per-keydedupeKeys (date-prefixed insights, per-firingalarm_fired:$firedAt, per-2h-bucket move nudge, and the ≥48h-cooldownsync_stalewhose date prefix always differs between fires). None rely on same-key repetition, so none regress.Tests
test/notification_dedupe_test.dart(injectsSharedPreferences.setMockInitialValues+ a fake present sink, no device):dedupeKeyemitted repeatedly → OS present happens exactly oncehigh_stressevent shape dedupes and now respects quiet hoursFiredKeyStorecaps at 200, evicts oldest, and a repeated record neither duplicates nor refreshes recencyThe stress-screen change itself is verified by inspection (routing
presentEvent→emit); the event-level test above exercises the exact event shape throughemit. A full widget test would need to mock the repository + the local-notifications plugin, which isn't worth the brittleness here.Verification note
No Flutter/Dart SDK or device was available in my environment, so this is verified by inspection + the new unit tests. On-device confirmation of the end-to-end fix is still wanted.
Closes #136
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Also fixes the repeated "Low readiness today" alert (same root cause).
Closes #138