Skip to content

Stop replayed chargingOn events re-notifying (#179) - #180

Merged
abdulsaheel merged 4 commits into
mainfrom
fix/charging-notification-replay
Aug 3, 2026
Merged

Stop replayed chargingOn events re-notifying (#179)#180
abdulsaheel merged 4 commits into
mainfrom
fix/charging-notification-replay

Conversation

@abdulsaheel

@abdulsaheel abdulsaheel commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

User description

Fixes #179 — repeated "Your band is on the charger" long after the charger was removed (Android, 0.9.21).

Root cause

The charging alert fires from a band event, and nothing on that path checked when the event happened.

  1. ble_engine.dart _absorbState sets state.charging from a decoded event and calls onState. The decoded event carries ts_epoch, and it was discarded.
  2. app_state.dart _onEngineState feeds every onState into DeviceAlerts.
  3. device_alerts.dart fired on any charging == true && _wasCharging != true, with _wasCharging held only in RAM.
  4. notification_service.dart re-posts id 1002 with no onlyAlertOnce, so each re-post re-buzzes.

The strap buffers its event log in flash, dumps it on connect, and re-sends events it has already delivered — so step 1 hands step 3 a chargingOn from hours ago and it is treated as a live plug-in. And because the edge state is RAM-only, every Android process restart re-arms it: EdgeApplication pre-warms a Dart engine on every process create, and KeepAliveWorker + the START_STICKY foreground service recreate the process routinely.

Evidence

Queried a real user export (openstrap_export_*.db, 6,980 events):

Check Result
Events delivered >10 min after they occurred 750
Events delivered >1 h after 410
Worst case 8h 26m stale (8-hour backlog dumped in ~90 s)
Duplicate (event_id, ts) deliveries 140 groups, up to 4× — different frame seq each time, so nothing upstream de-dupes them

The codebase already knew both facts: db.dart's insertEvent comments "the band re-sends events" (hence its ConflictAlgorithm.ignore), and GestureDispatcher carries a recency window plus a debounce for exactly this — "older than this = a drained/historical tap". Double-tap got that treatment; charging never did.

The fix

ChargeAlertPolicy (new, pure, unit-tested) — two guards, because neither is sufficient alone:

  • Recency — a chargingOn describing a plug-in from hours ago is history, not news. Catches first-time delivery of a backlogged event, which has no prior identity to match against.
  • Identity, persisted — never announce a charge session at or before the last one announced. Catches exact re-sends and restart-driven re-arming, both of which are recent enough to pass the recency check.

Supporting changes:

  • DeviceState.chargingTs carries the event's own strap timestamp. The flag itself still means "latest known charging state" and the UI should keep showing it — only consumers that treat the transition as live gate on the age.
  • DeviceAlerts persists its edge state, so "once per real event" survives a process restart. The low-battery hysteresis had the identical defect (re-armed on every restart) and is fixed with it.
  • chargingOff cancels the Charging card. It is a state claim, so leaving it in the tray after the puck comes off was wrong independently of this bug.
  • onlyAlertOnce on the device channel — explicitly not load-bearing. It only suppresses sound while a card with that id is still showing, so the user dismissing it re-arms the buzz. It just stops a redundant update from making noise; the real de-dupe is the policy.

Trade-off worth flagging

A genuine plug-in whose event is first delivered more than 15 min late no longer notifies. Real first-delivery lags in the export were 0 s, 105 s and 226 s (the app reconnecting minutes after the puck went on), so the window clears them with room to spare. A tighter window would have silenced those real alerts and fixed nothing, since re-sends are recent by definition.

A more precise alternative — debounce the charging edge until the event burst goes quiet, then act on the resolved final state — would preserve even a very-late first notification while still dropping a replayed on/off pair. It needs a timer and has its own failure mode (an off still queued for a later connection), so I left it out of a bug fix. Noted as a possible follow-up.

Not changed, deliberately

  • Engine state.charging still applies old events. Applying the replayed log in order converges to the correct current state; gating it would make the UI go blank on a legitimate plug-in the app learned about late, since there is no live charging source to fall back on.
  • HELLO's charging bit is parsed but unwired (control.dart:295 sets HelloInfo.charging; _absorbState reads only batteryPct/wristOn from it). A request/response bit would be a genuinely live signal, but the HELLO battery-block offsets are heuristic and unverified on hardware — not something to wire up in a bug fix. Worth a look separately.

Testing

test/device_alerts_test.dart — 23 new tests: policy table, the exact #179 loop (announce → remove → 4 re-sends → 5 process restarts ⇒ still one notification), the real measured lags still announcing, a genuinely new session after a restart still announcing, unset-RTC fallback, identity expiry so a backwards strap clock can't silence alerts forever, and low-battery hysteresis surviving a restart.

Each was checked to fail against the old behaviour — temporarily restoring the original rule fails 7 of them, including both #179 regression tests, and removing the restore guard fails the broken-store test.

  • flutter analyze — clean
  • flutter test --concurrency=11127 passed (1104 + 23)
  • No kAlgoVersion bump: notification layer only, no analytics output change.

Not verified on hardware — the policy is fully covered by unit tests, but onlyAlertOnce and the card-cancel are OS behaviours that need a device check: plug in → buzz; re-post while showing → silent update; unplug → card clears; plug in again → buzzes.


PR Type

Bug fix, Tests


Description


Diagram Walkthrough

flowchart LR
  A["BLE event\n(chargingOn + ts_epoch)"]
  B["DeviceState.chargingTs\n(strap timestamp)"]
  C["DeviceAlerts.onDeviceState\n(serialized async queue)"]
  D["ChargeAlertPolicy.evaluate\n(recency + identity guards)"]
  E["SharedPreferences\n(persisted edge state)"]
  F["NotificationService\n(idCharging / idLowBattery)"]

  A -- "ts_epoch carried" --> B
  B -- "chargingTs passed" --> C
  C -- "restore from" --> E
  C -- "evaluate verdict" --> D
  D -- "announce / stale / alreadyAnnounced / cooldown" --> C
  C -- "on announce: show/cancel" --> F
  C -- "write back" --> E
Loading

File Walkthrough

Relevant files
Bug fix
5 files
ble_engine.dart
Carry strap event timestamp into DeviceState.chargingTs   
+6/-0     
charge_alert_policy.dart
New pure policy: recency and identity guards for charging alerts
+148/-0 
device_alerts.dart
Persist edge state, serialize async queue, apply ChargeAlertPolicy
+177/-17
notification_service.dart
Add onlyAlertOnce for device notification category on Android
+8/-0     
app_state.dart
Pass chargingTs through to DeviceAlerts.onDeviceState       
+5/-1     
Enhancement
1 files
models.dart
Add chargingTs field to DeviceState                                           
+12/-0   
Tests
1 files
device_alerts_test.dart
Full test suite for charging and low-battery alert de-dupe
+377/-0 

Summary by CodeRabbit

  • New Features

    • Charging alerts now use event timestamps to prevent duplicate, stale, or replayed notifications.
    • Alert state persists across app restarts, including charging sessions and low-battery status.
    • Removing a device from charging cancels its active charging notification.
    • Low-battery alerts provide improved recovery and threshold handling.
  • Bug Fixes

    • Prevented repeated notification sounds when existing device alerts are updated.
    • Improved resilience when notification or alert-state storage operations fail.

The strap buffers its event log in flash, dumps it on connect, and re-sends
events it has already delivered. Nothing on the charging path checked WHEN an
event happened, so a chargingOn from hours earlier was processed as "the puck
just went on", and the false->true edge state lived only in RAM — which on
Android is reset constantly (EdgeApplication pre-warms an engine on every
process create; KeepAliveWorker and the START_STICKY FGS recreate the process
routinely). Each restart re-armed the alert and the next replay buzzed again.

Both halves are measurable in a real user export (6,980 events): 410 arrived
more than an hour after they occurred, worst case 8h26m — an 8-hour backlog
dumped in ~90s — and 140 distinct (event_id, ts) pairs were delivered more than
once, up to 4x, each with a different frame seq so nothing upstream de-dupes
them. db.dart's insertEvent already notes "the band re-sends events", and
GestureDispatcher already carries a recency window for the same reason
("older than this = a drained/historical tap"); the charging alert never got it.

- ChargeAlertPolicy (pure, unit-tested): a recency window rejects the stale
  backlog dump, and a persisted identity high-water rejects re-sends and
  restart-driven re-arming. Neither is sufficient alone — a re-send is recent,
  and a first-delivery stale event has no prior identity to match.
- DeviceState.chargingTs carries the event's own strap timestamp. The flag
  itself still means "latest known charging state" and the UI should keep
  showing it; only consumers treating the transition as live gate on the age.
- DeviceAlerts persists its edge state, so "once per real event" survives a
  process restart. Same defect applied to the low-battery hysteresis, which
  re-armed on every restart; fixed with it.
- chargingOff now cancels the Charging card. It is a state claim, so leaving it
  in the tray after the puck comes off was wrong anyway.
- onlyAlertOnce on the device channel, explicitly NOT load-bearing: it only
  suppresses sound while the card is still showing, so dismissing it re-arms.

Deliberate trade-off: a genuine plug-in first delivered more than 15 min late no
longer notifies. Measured real first-delivery lags were 0s, 105s and 226s, so
the window clears them with room to spare.

No kAlgoVersion bump: notification layer only, no analytics output changes.
@coderabbitai

coderabbitai Bot commented Aug 3, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@abdulsaheel, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 18 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 4ac52ebe-7e8a-4db2-b429-4bd655eab9e1

📥 Commits

Reviewing files that changed from the base of the PR and between 5710526 and 04dd2bf.

📒 Files selected for processing (2)
  • lib/notify/device_alerts.dart
  • test/device_alerts_test.dart
📝 Walkthrough

Walkthrough

The PR propagates strap charging timestamps into device state, adds timestamp-aware alert policy evaluation, persists alert edge state, isolates notification and storage failures, cancels alerts on charger removal, and prevents repeated Android notification sounds.

Changes

Charging alert lifecycle

Layer / File(s) Summary
Charging timestamp propagation
lib/data/models.dart, lib/ble/ble_engine.dart, lib/state/app_state.dart
DeviceState stores the latest charging event timestamp. BLE processing and app state pass this timestamp to DeviceAlerts.
Charging alert policy
lib/notify/charge_alert_policy.dart, test/device_alerts_test.dart
ChargeAlertPolicy validates event timestamps and returns verdicts for announcements, duplicates, stale events, persisted sessions, and cooldowns. Tests cover these policy decisions.
Alert state and notification handling
lib/notify/device_alerts.dart, lib/notify/notification_service.dart, test/device_alerts_test.dart
DeviceAlerts persists alert state, serializes updates, handles failures, cancels charging alerts on removal, and routes notifications through injectable seams. Android device notifications use onlyAlertOnce. Tests cover replay suppression, persistence, cancellation, low-battery hysteresis, and recovery.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant BleEngine
  participant AppState
  participant DeviceAlerts
  participant ChargeAlertPolicy
  participant DeviceAlertStore
  participant DeviceAlertSink
  BleEngine->>AppState: charging state and chargingTs
  AppState->>DeviceAlerts: onDeviceState(chargingTs)
  DeviceAlerts->>DeviceAlertStore: restore persisted state
  DeviceAlerts->>ChargeAlertPolicy: evaluate event timestamps
  ChargeAlertPolicy-->>DeviceAlerts: alert verdict
  DeviceAlerts->>DeviceAlertSink: show or cancel alert
  DeviceAlerts->>DeviceAlertStore: persist alert state
Loading

Possibly related PRs

  • OpenStrap/edge#137: Deduplicates insight notifications in NotificationCenter, while this PR deduplicates charging alerts in DeviceAlerts.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning The PR also adds low-battery state persistence, which is not required by the linked charging-notification issue [#179]. Move low-battery persistence changes to a separate PR unless the linked issue scope is expanded.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the fix for replayed chargingOn notifications, which is the primary change.
Linked Issues check ✅ Passed The changes address repeated Android charging notifications by suppressing replayed and stale events and canceling alerts on genuine removal [#179].
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit 04dd2bf)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Low-battery re-arm on stale chargingOn

A stale chargingOn (one that does not announce) still sets lowArmed = true at line 252 (if (announce) lowArmed = true). That is correct — only a genuine plug-in should re-arm. However, the _cancelledForOff = false reset at line 213 fires for ANY charging == true, including stale ones. This means a stale chargingOn re-opens the cancel latch, and the stale chargingOff that follows (which is NOT stale by the isStale check because its ts is also old) will then cancel the charging card. The test "a stale on/off pair does not swallow the next genuine plug-in" pins the cancel side, but the low-battery re-arm side has a subtler issue: a stale chargingOn followed by a stale chargingOff leaves _wasCharging = false, which means the next genuine plug-in is NOT suppressed by alreadyCharging — that part is correct. The actual concern is that lowArmed is re-armed only on announce, so a stale plug-in does NOT re-arm it. But the test at line 500 ("a replayed plug-in does not re-arm the low alert") passes because announce is false for a stale event. This appears correct on inspection, but the interaction between _cancelledForOff being reset on any charging == true (stale or not) and the cancel guard !ChargeAlertPolicy.isStale(chargingTs, wallNowSec: nowSec) means a stale chargingOn followed immediately by a live chargingOff will NOT cancel the card (because _cancelledForOff was set to false by the stale on, then the live off sets it to true and cancels — actually this is fine). The real gap: when chargingTs is null on a charging == false update, isStale(null, ...) returns false, so cancelChargingCard fires even though the timestamp is unknown. This could cancel a live charging card on a chargingOff event with no timestamp, which is the same replay problem for the off direction.

final cancelChargingCard = charging == false &&
    !_cancelledForOff &&
    !ChargeAlertPolicy.isStale(chargingTs, wallNowSec: nowSec);
if (cancelChargingCard) _cancelledForOff = true;
// Any chargingOn — even one too stale to announce — means a card may exist
// again, so the next removal must be free to clear it.
if (charging == true) _cancelledForOff = false;
Low-battery latch not durable on store failure

The _kLowArmed key is written to the store whenever lowArmedChanged is true, but it is read back in _restore() and applied to _lowArmed. However, _lowArmed starts as true in RAM, and the restore only overrides it if armed != null. If the store write of _kLowArmed = 0 (disarmed) succeeds but a subsequent process restart reads it back correctly, that is fine. The problem is the opposite: if the store write fails (throwing store), _lowArmed is set to false in RAM (line 260) before the write, so within the same process re-sends are correctly suppressed. But after a restart, the store has no record of the disarm, so _lowArmed defaults back to true and the low-battery alert fires again. The test "the once-per-drain guard survives a process restart" at line 447 passes only because the fake store does NOT throw — it does not cover the throwing-store + restart combination for low battery. This is the same class of bug as #179 but for the low-battery path: a throwing store leaves the latch un-persisted, and a restart re-arms it.

if (lowArmedChanged) {
  await _io(() => _store.writeInt(_kLowArmed, lowArmed ? 1 : 0));
}
Untimed cooldown bypass on first announce

When eventTsEpoch is unusable (null or below kMinPlausibleUnix) and lastAnnouncedWallSec is null (no prior announcement), the policy returns announce. This is intentional for the very first plug-in. However, after an untimed announcement, _lastAnnouncedEventTs is set to null and persistEventTs = 0 is written. On a process restart, _restore() reads eventTs = 0 and maps it to null (line 146: (eventTs != null && eventTs > 0) ? eventTs : null), and reads wallSec correctly. So the cooldown path works correctly across restarts for the untimed case. No bug here — confirming the logic is sound. However, there is a subtle issue: if the store write of _kLastChargeWall succeeds but the write of _kLastChargeTs (value 0) fails, after a restart lastAnnouncedWallSec is set but lastAnnouncedEventTs is null — which is the correct state for the untimed path. So the failure mode is benign. This is low confidence and likely not a real issue given the independent _io calls.

if (!timestampUsable(eventTsEpoch, wallNowSec: wallNowSec)) {
  // No usable strap clock → we cannot tell news from replay. Fall back to a
  // blunt wall-clock cooldown rather than guessing.
  if (lastAnnouncedWallSec != null &&
      wallNowSec - lastAnnouncedWallSec < untimedCooldownSec) {
    return ChargeAlertVerdict.cooldown;
  }
  return ChargeAlertVerdict.announce;

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Latest suggestions up to 04dd2bf

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Persist identity before showing notification

The persistence writes happen after the notification show, so if the platform plugin
throws, the store is never written and the session re-announces after the next
process restart — the exact bug this PR fixes. Per the AGENTS.md §4.3 pattern
("commit before ACK"), the identity state must be persisted before (or independently
of) the presentation step. Move the _store.writeInt calls before the _notes.show
call so a throwing sink cannot leave the store un-written.

lib/notify/device_alerts.dart [273-285]

 if (announce) {
+  await _io(() => _store.writeInt(_kLastChargeWall, nowSec));
+  if (persistEventTs != null) {
+    await _io(() => _store.writeInt(_kLastChargeTs, persistEventTs!));
+  }
   await _io(() => _notes.show(
         id: NotificationService.idCharging,
         title: 'Charging',
         body: 'Your band is on the charger.',
       ));
-  await _io(() => _store.writeInt(_kLastChargeWall, nowSec));
-  if (persistEventTs != null) {
-    await _io(() => _store.writeInt(_kLastChargeTs, persistEventTs!));
-  }
   // A real plug-in clears any stale low alert.
   await _io(() => _notes.cancel(NotificationService.idLowBattery));
 }
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that if _notes.show throws, the _store.writeInt calls are never reached, potentially causing re-announcement after restart. However, the PR's own comment explicitly states "neither step reads the other's result — so all of them are attempted independently" via _io which absorbs failures, and the test a failing sink still lets the announcement persist validates this exact scenario works. The ordering concern is real but the _io isolation already addresses it partially.

Medium
Prevent stale chargingOff from cancelling live card

When charging == null (a battery-only update with no charging field),
_cancelledForOff is neither set nor cleared. This is correct for the cancel latch.
However, when a stale chargingOn arrives (charging == true), _cancelledForOff is
reset to false, meaning the very next chargingOff — even a stale one from the same
backlog — will pass the !_cancelledForOff check and attempt to cancel the card. The
isStale guard on the cancel path does protect against this, but only if chargingTs
is usable. If the stale chargingOff carries an unusable timestamp (null or below the
plausible floor), isStale returns false and the card gets cancelled incorrectly. The
reset of _cancelledForOff should only happen for a non-stale chargingOn (i.e., one
that could have produced a card), mirroring the staleness check on the cancel side.

lib/notify/device_alerts.dart [207-213]

 final cancelChargingCard = charging == false &&
     !_cancelledForOff &&
     !ChargeAlertPolicy.isStale(chargingTs, wallNowSec: nowSec);
 if (cancelChargingCard) _cancelledForOff = true;
-// Any chargingOn — even one too stale to announce — means a card may exist
-// again, so the next removal must be free to clear it.
-if (charging == true) _cancelledForOff = false;
+// Reset the cancel latch only for a chargingOn that could have produced a
+// card (i.e., not a stale backlog replay). A stale on followed by a stale
+// off with an unusable timestamp would otherwise cancel a live card.
+if (charging == true &&
+    !ChargeAlertPolicy.isStale(chargingTs, wallNowSec: nowSec)) {
+  _cancelledForOff = false;
+}
Suggestion importance[1-10]: 6

__

Why: The suggestion identifies a real edge case: a stale chargingOn resets _cancelledForOff, and a subsequent chargingOff with an unusable timestamp (where isStale returns false) could incorrectly cancel a live card. This is a subtle but genuine bug in the interaction between the two latches that the existing tests may not cover.

Low
General
Prevent memoised restore future from silencing all alerts

The catchError on the outer queue chain catches errors from _restore() as well as
_apply(). If _restore() throws despite the internal try/catch (e.g. a future
implementation change), the error is swallowed by catchError but _restored is now a
completed-with-error future. Because _restored is memoised, every subsequent call to
onDeviceState will await the same failed future, which will re-throw and be
swallowed again — silently killing all alerts for the process lifetime. The
_restore() method's own try/catch already handles this, but the memoisation of a
potentially-errored future is a latent hazard. Ensure _restore never completes with
an error by wrapping its entire body so the memoised future is always a success.

lib/notify/device_alerts.dart [135-153]

-void onDeviceState({double? batteryPct, bool? charging, int? chargingTs}) {
-  final nowSec = DateTime.now().millisecondsSinceEpoch ~/ 1000;
-  _queue = _queue.then((_) async {
-    _restored ??= _restore();
-    await _restored;
-    await _apply(
-      batteryPct: batteryPct,
-      charging: charging,
-      chargingTs: chargingTs,
-      nowSec: nowSec,
-    );
-  }).catchError((_) {
-    // An alert is a nicety; it must never break the state pipeline, and a
-    // throw must not poison the queue for every later update.
-  });
+Future<void> _restore() async {
+  try {
+    final eventTs = await _store.readInt(_kLastChargeTs);
+    final wallSec = await _store.readInt(_kLastChargeWall);
+    final armed = await _store.readInt(_kLowArmed);
+    _lastAnnouncedEventTs = (eventTs != null && eventTs > 0) ? eventTs : null;
+    _lastAnnouncedWallSec = wallSec;
+    if (armed != null) _lowArmed = armed != 0;
+  } catch (_) {
+    // Degrade to defaults — costs at most one duplicate announcement.
+    _lastAnnouncedEventTs = null;
+    _lastAnnouncedWallSec = null;
+    // Do NOT rethrow: _restored is memoised; a thrown error here would be
+    // re-awaited and re-thrown by every later onDeviceState call, silently
+    // killing all alerts for the process lifetime.
+  }
 }
Suggestion importance[1-10]: 2

__

Why: The _restore() method already has a comprehensive try/catch that catches all exceptions and falls back to defaults without rethrowing. The suggestion's improved_code is essentially identical to the existing code with only an added comment, making this a documentation-only change with no functional difference.

Low

Previous suggestions

Suggestions up to commit a32e92f
CategorySuggestion                                                                                                                                    Impact
Possible issue
Persist charge state before presenting notification

The _notes.show(...) call can throw (e.g. the platform plugin is unavailable in a
headless wake), and the catchError in onDeviceState silently swallows it. If show
throws before _store.writeInt(_kLastChargeWall, ...) executes, the in-memory latches
(_lastAnnouncedWallSec, _lastAnnouncedEventTs, _wasCharging) are already committed
but the persisted state is not written, so a process restart will re-announce the
same session — exactly the bug this PR fixes. The store writes should be attempted
regardless of whether the notification presentation succeeds, using a try/finally or
by reordering so the persist comes before the show.

lib/notify/device_alerts.dart [266-278]

 if (announce) {
+  // Persist BEFORE presenting: if show() throws the store already records
+  // this session, so a process restart won't re-announce it.
+  await _store.writeInt(_kLastChargeWall, nowSec);
+  if (persistEventTs != null) {
+    await _store.writeInt(_kLastChargeTs, persistEventTs);
+  }
   await _notes.show(
     id: NotificationService.idCharging,
     title: 'Charging',
     body: 'Your band is on the charger.',
   );
-  await _store.writeInt(_kLastChargeWall, nowSec);
-  if (persistEventTs != null) {
-    await _store.writeInt(_kLastChargeTs, persistEventTs);
-  }
   // A real plug-in clears any stale low alert.
   await _notes.cancel(NotificationService.idLowBattery);
 }
Suggestion importance[1-10]: 7

__

Why: If _notes.show() throws before the _store.writeInt calls execute, the in-memory latches are committed but the persisted state is not, meaning a process restart could re-announce the same session. Reordering to persist before presenting is a valid defensive improvement, though the catchError in onDeviceState and the in-memory latches provide partial protection.

Medium
Fix plug-in re-arm precedence over low-battery disarm

When announce is true and charging == true, lowArmed is set to true and _lowArmed is
updated, but the _kLowArmed store write only fires when lowArmedChanged is true. If
_lowArmed was already true (e.g. the battery had recovered past _rearmPct earlier in
this process), the store is not written — which is fine. However, if fireLow sets
lowArmed = false AND announce is also true in the same call (charging just started
at low battery), lowArmed ends up false after the if (fireLow) lowArmed = false
line, which means the plug-in does NOT re-arm the latch. The announce re-arm should
come after fireLow to take precedence.

lib/notify/device_alerts.dart [251-260]

 var lowArmed = _lowArmed;
-if (announce) lowArmed = true;
 if (batteryPct != null && batteryPct >= _rearmPct) lowArmed = true;
 final fireLow = batteryPct != null &&
     charging != true &&
     batteryPct < _lowPct &&
     lowArmed;
 if (fireLow) lowArmed = false;
+// A genuine plug-in re-arms after a drain — evaluated last so it wins over
+// fireLow in the same update (e.g. charging started while still at 12%).
+if (announce) lowArmed = true;
 final lowArmedChanged = lowArmed != _lowArmed;
 _lowArmed = lowArmed;
Suggestion importance[1-10]: 6

__

Why: In the edge case where announce is true and fireLow is also true (charging started while battery is below 15%), the current code sets lowArmed = false after the announce re-arm, leaving the latch disarmed when it should be re-armed. Moving if (announce) lowArmed = true after if (fireLow) lowArmed = false correctly gives the plug-in re-arm precedence.

Low
General
Prevent stale chargingOn from re-arming card-cancel latch

_cancelledForOff is reset to false on any charging == true event, including stale
ones. The comment says "even one too stale to announce — means a card may exist
again", but a stale chargingOn cannot have posted a card (the announce path is
blocked), so resetting _cancelledForOff on a stale chargingOn incorrectly allows the
next chargingOff (even a fresh one) to cancel a card that was never posted by this
session. The reset should be conditioned on a non-stale chargingOn, consistent with
the announce flag.

lib/notify/device_alerts.dart [207-213]

 final cancelChargingCard = charging == false &&
     !_cancelledForOff &&
     !ChargeAlertPolicy.isStale(chargingTs, wallNowSec: nowSec);
 if (cancelChargingCard) _cancelledForOff = true;
-// Any chargingOn — even one too stale to announce — means a card may exist
-// again, so the next removal must be free to clear it.
-if (charging == true) _cancelledForOff = false;
+// Only a non-stale chargingOn can have posted a card, so only then must the
+// next removal be free to clear it.
+if (charging == true &&
+    !ChargeAlertPolicy.isStale(chargingTs, wallNowSec: nowSec)) {
+  _cancelledForOff = false;
+}
Suggestion importance[1-10]: 5

__

Why: A stale chargingOn cannot post a card (the announce path is blocked), so resetting _cancelledForOff on a stale event incorrectly allows a subsequent chargingOff to cancel a card that was never posted. However, the practical impact is limited since cancelling a non-existent notification is typically a no-op on most platforms.

Low
Suggestions up to commit 5710526
CategorySuggestion                                                                                                                                    Impact
Possible issue
Clear stale identity timestamp on untimed announcement

When announce is true but chargingTs is not usable (unset RTC path),
_lastAnnouncedWallSec is updated but _lastAnnouncedEventTs is left at its old value.
On the next call, ChargeAlertPolicy.evaluate will see a non-null
lastAnnouncedEventTs from a previous session alongside the fresh
lastAnnouncedWallSec, and the identity check (ts <= lastAnnouncedEventTs) may
incorrectly suppress a genuinely new session whose timestamp happens to be older
than the stored high-water mark. _lastAnnouncedEventTs should be explicitly cleared
to null when announcing without a usable timestamp, so the identity check is
disabled for that announcement.

lib/notify/device_alerts.dart [213-219]

 if (announce) {
   _lastAnnouncedWallSec = nowSec;
   if (ChargeAlertPolicy.timestampUsable(chargingTs, wallNowSec: nowSec)) {
     _lastAnnouncedEventTs = chargingTs;
     persistEventTs = chargingTs;
+  } else {
+    _lastAnnouncedEventTs = null;
   }
 }
Suggestion importance[1-10]: 7

__

Why: This is a valid edge case: when announcing via the untimed (cooldown) path, _lastAnnouncedEventTs retains its old value, which could cause the identity check to incorrectly suppress a future session with a timestamp older than the stored high-water mark. Clearing it to null on untimed announcements correctly disables the identity check for that path.

Medium
Remove non-persisted latch that blocks real card cancellation

The _cancelledForOff latch is never persisted. After an Android process restart,
_cancelledForOff resets to false, so a fresh process receiving a replayed stale
chargingOff event will call _notes.cancel(idCharging) even though the card was
already cleared in the previous process — and more critically, it will set
_cancelledForOff = true, which then blocks the cancel for the real removal that
follows. This is the same RAM-only latch class as the original _wasCharging bug. The
_cancelledForOff state should be persisted alongside the other edge state, or the
cancel should be made unconditional for non-stale chargingOff events (idempotent
cancels are harmless).

lib/notify/device_alerts.dart [194-200]

+// cancel(id) is idempotent — calling it when no card is showing costs nothing.
+// Removing the _cancelledForOff latch avoids a RAM-only state that resets on
+// every Android process restart and blocks the real removal after a replay.
 final cancelChargingCard = charging == false &&
-    !_cancelledForOff &&
     !ChargeAlertPolicy.isStale(chargingTs, wallNowSec: nowSec);
-if (cancelChargingCard) _cancelledForOff = true;
-// Any chargingOn — even one too stale to announce — means a card may exist
-// again, so the next removal must be free to clear it.
-if (charging == true) _cancelledForOff = false;
Suggestion importance[1-10]: 7

__

Why: The _cancelledForOff latch is not persisted, so after a process restart it resets to false. A replayed stale chargingOff sets it to true, which then blocks the cancel for the real removal. Since cancel is idempotent, removing the latch and making the cancel unconditional (for non-stale events) is safer and avoids this RAM-only state class bug.

Medium
Persist announcement record before showing notification

_store.writeInt(_kLastChargeWall, nowSec) is called after _notes.show(...). If
_notes.show throws (e.g. platform plugin unavailable in a headless wake), the
wall-clock timestamp is never persisted, so _lastAnnouncedWallSec is updated in RAM
but not on disk. On the next process restart the identity/cooldown guard has no
record of the announcement and the alert fires again — the exact bug this PR fixes
for the RAM-only case. The store writes should happen before the notification show,
consistent with the "commit before ACK" ordering principle in AGENTS.md §3.1.

lib/notify/device_alerts.dart [242-253]

 if (announce) {
+  await _store.writeInt(_kLastChargeWall, nowSec);
+  if (persistEventTs != null) {
+    await _store.writeInt(_kLastChargeTs, persistEventTs);
+  }
   await _notes.show(
     id: NotificationService.idCharging,
     title: 'Charging',
     body: 'Your band is on the charger.',
   );
-  await _store.writeInt(_kLastChargeWall, nowSec);
-  if (persistEventTs != null) {
-    await _store.writeInt(_kLastChargeTs, persistEventTs);
-  }
   // A real plug-in clears any stale low alert.
   await _notes.cancel(NotificationService.idLowBattery);
 }
Suggestion importance[1-10]: 6

__

Why: If _notes.show throws, the store writes are skipped and _lastAnnouncedWallSec is updated in RAM but not persisted. On process restart, the identity/cooldown guard has no record and could re-fire. However, the catchError in onDeviceState swallows errors, and the in-process latches (committed before any await) already prevent same-process re-fires, so the risk is limited to cross-restart scenarios.

Low
Suggestions up to commit f094b79
CategorySuggestion                                                                                                                                    Impact
General
Restore persisted state atomically to avoid inconsistent defaults

The _restore() method swallows all exceptions, but _restored is memoised as the
Future returned by _restore(). If _restore() itself completes normally (the catch
ensures it always does), this is fine — but if a partial read succeeds before a
throw (e.g. _kLastChargeTs reads successfully, then _kLastChargeWall throws),
_lastAnnouncedEventTs is set while _lastAnnouncedWallSec remains null. This leaves
the identity check in an inconsistent state: identityLive will be true (no
lastAnnouncedWallSec) so the identity guard is skipped, but lastAnnouncedEventTs is
set, meaning a replay with ts <= lastAnnouncedEventTs will still be caught. The real
risk is the inverse: _lastAnnouncedWallSec set but _lastAnnouncedEventTs null, which
disables the identity check entirely for timed events. Reset both fields atomically
on any read failure to guarantee a consistent default state.

lib/notify/device_alerts.dart [122-129]

 Future<void> _restore() async {
   try {
-    _lastAnnouncedEventTs = await _store.readInt(_kLastChargeTs);
-    _lastAnnouncedWallSec = await _store.readInt(_kLastChargeWall);
+    final eventTs = await _store.readInt(_kLastChargeTs);
+    final wallSec = await _store.readInt(_kLastChargeWall);
     final armed = await _store.readInt(_kLowArmed);
+    // Assign atomically so the identity check always sees a consistent pair.
+    _lastAnnouncedEventTs = eventTs;
+    _lastAnnouncedWallSec = wallSec;
     if (armed != null) _lowArmed = armed != 0;
-  } catch (_) {}
+  } catch (_) {
+    // Degrade to defaults; a duplicate alert is preferable to broken state.
+    _lastAnnouncedEventTs = null;
+    _lastAnnouncedWallSec = null;
+  }
 }
Suggestion importance[1-10]: 5

__

Why: The suggestion correctly identifies that partial reads could leave _lastAnnouncedEventTs and _lastAnnouncedWallSec in an inconsistent state if an exception occurs mid-restore. The fix of reading into local variables first and assigning atomically is a genuine improvement, though the practical risk is low since SharedPreferences.getInt rarely throws mid-sequence.

Low
Update edge state before async notification calls

_wasCharging is only updated after both the charging == false and charging == true
branches, but the charging == false cancel fires whenever _wasCharging != false
including when _wasCharging is null (fresh process). This means a process restart
while the charger is disconnected will cancel the charging notification card on the
very first update even though no card is showing, which is harmless, but more
critically the _wasCharging update at the bottom is the only reset on the failure
path. If _apply throws between the charging == true block and if (charging != null)
_wasCharging = charging, the flag is never updated and the next call re-enters the
announce branch. The _wasCharging assignment should be moved to immediately after
the verdict check (before the async notification calls) so the in-process edge state
is always updated regardless of whether the notification succeeds.

lib/notify/device_alerts.dart [169-198]

 if (charging == false && _wasCharging != false) {
   await _notes.cancel(NotificationService.idCharging);
 }
 
 if (charging == true) {
   final verdict = ChargeAlertPolicy.evaluate(
-    ...
+    eventTsEpoch: chargingTs,
+    wallNowSec: nowSec,
+    wasCharging: _wasCharging,
+    lastAnnouncedEventTs: _lastAnnouncedEventTs,
+    lastAnnouncedWallSec: _lastAnnouncedWallSec,
   );
   if (verdict == ChargeAlertVerdict.announce) {
-    ...
+    await _notes.show(
+      id: NotificationService.idCharging,
+      title: 'Charging',
+      body: 'Your band is on the charger.',
+    );
+    _lastAnnouncedWallSec = nowSec;
+    await _store.writeInt(_kLastChargeWall, nowSec);
+    if (ChargeAlertPolicy.timestampUsable(chargingTs, wallNowSec: nowSec)) {
+      _lastAnnouncedEventTs = chargingTs;
+      await _store.writeInt(_kLastChargeTs, chargingTs!);
+    }
     await _notes.cancel(NotificationService.idLowBattery);
     await _setLowArmed(true);
   }
 }
+// Update edge state immediately, before any further awaits, so the
+// in-process guard is always current even if a notification call throws.
 if (charging != null) _wasCharging = charging;
Suggestion importance[1-10]: 4

__

Why: The suggestion raises a valid concern about _wasCharging being updated after async calls, but the actual risk is low because _apply is already wrapped in a catchError at the queue level, and the ChargeAlertPolicy.evaluate check with wasCharging already guards against re-entry. The improved_code is essentially identical to the existing code with just a comment added, making the practical impact minimal.

Low

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 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/charge_alert_policy.dart`:
- Around line 79-81: Update ChargeAlertPolicy.timestampUsable() to use the
shared kMinPlausibleUnix constant from sync_policy.dart instead of defining
plausibleUnixFloor locally; import or relocate the constant so both charge-alert
and sync clock policies reference one shared value.

In `@lib/notify/device_alerts.dart`:
- Around line 41-54: Update _NotificationServiceSink.show to route device alerts
through NotificationCenter.emit using a NotificationEvent with
NotifCategory.device, preserving the existing device-alert test seam. Ensure the
production path receives the emitter’s quiet-hour guard, fire-once claim/release
behavior, presentation-chain locking, and history key, while leaving
ChargeAlertPolicy responsible only for charge-specific replay and low-water
replay suppression.
- Around line 169-171: Update the cancellation guard around
_notes.cancel(NotificationService.idCharging) to validate the charging-off event
via DeviceState.chargingTs, using the same recency check as the show path.
Cancel only for a usable recent timestamp, or for an unusable timestamp when no
announced charging session remains live; ignore replayed historical events while
preserving the existing charging transition check.

In `@test/device_alerts_test.dart`:
- Around line 107-110: Add coverage in the existing backlog replay tests for an
event age greater than implausibleAgeSec (86400 seconds), asserting the
documented untimed announce verdict when no prior announcement exists. Also add
a regression test covering a replayed chargingOff event during an active
session, verifying it does not cancel the Charging card; use the existing
verdict/test helpers and device-alert behavior symbols in
test/device_alerts_test.dart and lib/notify/device_alerts.dart.
🪄 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: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: cbf205bc-16a3-4490-a2ff-b14b1f7f81dc

📥 Commits

Reviewing files that changed from the base of the PR and between 4cc0d9b and f094b79.

📒 Files selected for processing (7)
  • lib/ble/ble_engine.dart
  • lib/data/models.dart
  • lib/notify/charge_alert_policy.dart
  • lib/notify/device_alerts.dart
  • lib/notify/notification_service.dart
  • lib/state/app_state.dart
  • test/device_alerts_test.dart

Comment thread lib/notify/charge_alert_policy.dart Outdated
Comment on lines +41 to +54
class _NotificationServiceSink implements DeviceAlertSink {
const _NotificationServiceSink();

@override
Future<void> show({
required int id,
required String title,
required String body,
}) =>
NotificationService.instance.showDevice(id: id, title: title, body: body);

@override
Future<void> cancel(int id) => NotificationService.instance.cancel(id);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Compare the emitter contract with the direct showDevice bypass.
set -euo pipefail

echo "== NotificationCenter.emit surface =="
ast-grep outline lib/notify/notification_center.dart --items all

echo "== callers of showDevice =="
rg -nP -C4 '\bshowDevice\s*\(' --glob 'lib/**/*.dart'

echo "== preference gating inside the emitter =="
rg -nP -C4 'NotificationPrefs|quietStartMin|quietEndMin|categoryEnabled' --glob 'lib/notify/notification_center.dart'

Repository: OpenStrap/edge

Length of output: 268


🏁 Script executed:

#!/bin/bash
set -eo pipefail

echo "== repository notifications files =="
git ls-files 'lib/notify/**/*.dart'

echo "== device_alerts outline =="
ast-grep outline lib/notify/device_alerts.dart --items all || true

echo "== notification_center outline =="
ast-grep outline lib/notify/notification_center.dart --items all || true

echo "== notification_service outline =="
ast-grep outline lib/notify/notification_service.dart --items all || true

echo "== search notify identifiers =="
rg -n -C3 'DeviceAlertSink|_NotificationServiceSink|NotificationCenter|emit|showDevice|presentEvent|dedupe|preference|quiet|ChargeAlertPolicy' --glob 'lib/notify/**/*.dart' || true

Repository: OpenStrap/edge

Length of output: 463


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== lib files under git =="
git ls-files lib | sed -n '1,200p'

echo "== locate relevant files by name =="
fd -a 'device_alerts|notification_center|notification_service|notification_pref|charge_alert' . | sed -n '1,120p'

echo "== current branch status =="
git status --short

Repository: OpenStrap/edge

Length of output: 5981


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== device_alerts.dart =="
sed -n '1,240p' lib/notify/device_alerts.dart

echo "== notification_center.dart =="
sed -n '1,320p' lib/notify/notification_center.dart

echo "== notification_service.dart =="
sed -n '1,260p' lib/notify/notification_service.dart

echo "== notification_prefs.dart =="
sed -n '1,240p' lib/notify/notification_prefs.dart

echo "== charge_alert_policy.dart =="
sed -n '1,220p' lib/notify/charge_alert_policy.dart

echo "== identifiers =="
rg -n -C3 'DeviceAlertSink|_NotificationServiceSink|showDevice|presentEvent|NotificationCenter\.emit|emit\(|dedupe|persistent|locking|lock|race|charging|low-battery|low battery|ChargeAlertPolicy|show charging|charging alert|battery|battery alert' --glob 'lib/**/*.dart' --glob 'test/**/*.dart' || true

Repository: OpenStrap/edge

Length of output: 46173


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== rest of notification_service.dart =="
sed -n '240,540p' lib/notify/notification_service.dart

echo "== notification_ids.dart =="
sed -n '1,240p' lib/notify/notification_ids.dart

echo "== notification_event.dart =="
sed -n '1,240p' lib/notify/notification_event.dart

echo "== test references =="
sed -n '1,320p' test/device_alerts_test.dart
echo "== =="
sed -n '1,360p' test/notification_center_test.dart

Repository: OpenStrap/edge

Length of output: 34446


Route device alerts through NotificationCenter.emit.

DeviceAlerts._apply calls _notes.show(...), and _NotificationServiceSink.show() calls NotificationService.instance.showDevice directly. That bypasses NotificationCenter.emit’s category quiet-hour guard, fire-once dedupe claim, release-on-unpresent behavior, and the presentation-chain lock, and these alerts miss the notification history key that belongs with the emitter. Keep the device alert test seam, but make the production sink go through NotificationCenter.emit with a NotificationEvent using NotifCategory.device; keep ChargeAlertPolicy responsible only for charge-specific replay/low-water replay suppression.

🤖 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/notify/device_alerts.dart` around lines 41 - 54, Update
_NotificationServiceSink.show to route device alerts through
NotificationCenter.emit using a NotificationEvent with NotifCategory.device,
preserving the existing device-alert test seam. Ensure the production path
receives the emitter’s quiet-hour guard, fire-once claim/release behavior,
presentation-chain locking, and history key, while leaving ChargeAlertPolicy
responsible only for charge-specific replay and low-water replay suppression.

Source: Coding guidelines

Comment thread lib/notify/device_alerts.dart Outdated
Comment thread test/device_alerts_test.dart
Bot review follow-up. Four of seven findings were real; verified each against
code before touching anything.

- Reuse kMinPlausibleUnix from sync_policy.dart instead of a second copy of the
  1700000000 floor (CodeRabbit). ble_state.dart already imports from there.

- Remove the 24h usability cap. It was modelled on GestureDispatcher's
  _plausibleAgeCapSec, which is right for a tap (bounded backlog) and wrong
  here: the strap banks days, so a two-day-old chargingOn is a real old event,
  not a broken clock — and the cap routed it to the untimed path, where it
  ANNOUNCED. That reintroduced the exact #179 bug past 24h. An unset RTC reads
  near zero and is still caught by the floor. Prompted by CodeRabbit asking for
  a test pinning the old behaviour; the behaviour was wrong, so the test pins
  the fix instead.

- Don't let a replayed chargingOff clear a live Charging card (CodeRabbit).
  Writing this exposed a second defect in my own first attempt: the cancel was
  keyed off the charging TRANSITION, but a stale chargingOff still has to
  update _wasCharging (it is the latest known state), which consumed the
  transition and left the REAL removal with nothing to act on — the card would
  strand in the tray. Cancel now keys off a separate "already cancelled for
  this off-state" latch.

- Decide-then-act in _apply, so every latch is committed before the first await
  (PR Agent, citing AGENTS.md 4.3 — a real repo convention, verified). A
  throwing sink previously left _lowArmed armed and retried the alert on every
  update, and a throwing store could re-announce the same charge session; the
  queue's catchError made both silent. Restore now assigns the identity pair
  atomically too.

Declined, with reasons:

- Routing device alerts through NotificationCenter.emit (CodeRabbit). Its only
  added gate for this category is quiet hours — notification_prefs.dart has
  `NotifCategory.device => true, // device alerts aren't user-gated here`, so
  there is no category toggle being bypassed. It would silence a confirmation
  of an action the user just performed, during exactly the window people put a
  band on the charger. Its fire-once guard is set-membership on a dedupeKey and
  cannot express the `<=` identity rule this needs.

- Persist-before-announce and untimed-cooldown durability gaps (PR Agent): both
  require SharedPreferences writes to fail, where the recency window still
  holds. The in-memory latches now cover the same window regardless.

Tests 1127 -> 1133. Each new test verified to fail against the pre-fix code.
@abdulsaheel

Copy link
Copy Markdown
Collaborator Author

Reviewed both bots against the code. 4 of 7 findings were real and are fixed in 5710526; 3 declined with reasons below.

Fixed

1. Duplicate plausible-unix floor (CodeRabbit) — real. kMinPlausibleUnix = 1700000000 already exists in sync_policy.dart:55, and ble_state.dart:13 already imports from there. Now imported rather than re-declared.

2. The 24 h usability cap was itself a bug (surfaced by CodeRabbit asking for a test at age > implausibleAgeSec). Writing that test showed the behaviour it would have pinned is wrong: the cap was modelled on GestureDispatcher._plausibleAgeCapSec, which is right for a tap (bounded backlog) and wrong here — the strap banks days, so a two-day-old chargingOn is a real old event, not a broken clock. The cap routed it to the untimed path where it announced, reintroducing #179 past 24 h. Cap removed; an unset RTC reads near zero and is still caught by the floor. The test pins the fix instead of the bug.

3. A replayed chargingOff must not clear a live Charging card (CodeRabbit) — real. Writing the test also exposed a second defect in my own first attempt: I keyed the cancel off the charging transition, but a stale chargingOff still has to update _wasCharging (it is the latest known state), which consumed the transition — so the real removal that followed found nothing to act on and stranded the card in the tray. The cancel now keys off a separate "already cancelled for this off-state" latch. Both paths covered.

4. Latch ordering (PR Agent, citing AGENTS.md §4.3) — real, and the citation checks out: §4.3 "Sticky boolean latches never reset on the failure path" is a documented recurring class in this repo. _apply is now decide-then-act, every latch committed before the first await. A throwing sink previously left _lowArmed armed and retried on every update; a throwing store could re-announce the same session — and the queue's catchError made both silent. Restore also assigns the identity pair atomically.

Declined

Route device alerts through NotificationCenter.emit (CodeRabbit). Checked what it would actually add for this category: notification_prefs.dart:120 reads NotifCategory.device => true, // device alerts aren't user-gated here, so no category toggle is being bypassed — the only added gate is quiet hours. That would silence a confirmation of an action the user just performed, during exactly the window in which people put a band on the charger (before bed). Its fire-once guard is also set-membership on a dedupeKey via FiredKeyStore, which cannot express the <= identity comparison this needs. I don't think §4.6's "bypassing the prefs gate" applies when the prefs gate is a literal true for this category — but happy to revisit if a device toggle ever lands.

Persist-before-announce durability gap and untimed cooldown with no prior wall record (PR Agent). Both require a SharedPreferences write to fail. In that window the recency check still rejects anything stale, and after this change the in-memory latches cover the same session regardless of whether persistence succeeded. Narrow enough that I'd rather not add code for it.

One correction to a PR Agent suggestion I did partly take: it argued "the real risk is the inverse — _lastAnnouncedWallSec set but _lastAnnouncedEventTs null". That specific direction is unreachable, because _kLastChargeTs is read first. I made the assignment atomic anyway so the invariant is local rather than dependent on statement order, and noted why in the comment.

flutter analyze clean; 1133 tests (1127 → 1133). Each new test was confirmed to fail against the pre-fix code — restoring the 24 h cap fails 2, and moving the low-battery disarm back after the await fails the latch test.

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 5710526

Second review round. One of four findings real.

A session announced with no usable strap clock has no identity of its own, but
the previous high-water was left in place — so it went on to judge a session it
knew nothing about, and the next usable timestamp falling at or below it was
suppressed. That direction costs the user a real "on the charger" confirmation,
which is worse than the duplicate this PR set out to kill. Cleared in memory and
persisted as 0 (every real strap timestamp is far above it), so the untimed path
is guarded only by the wall-clock cooldown, which is what ChargeAlertPolicy's
untimed branch already assumes.

Declined:

- "Low-armed not persisted" / "untimed cooldown not restart-safe" — both require
  SharedPreferences writes to fail. Not a regression: before this PR the latch
  was RAM-only and re-armed on EVERY process start, so a dead store degrades to
  exactly the old behaviour and a working one is strictly better. There is no
  way to remember a latch without somewhere to remember it. Noted the asymmetry
  in a comment: charging keeps the recency window as a second line of defence,
  low battery has no timestamp to reason about.

- "Remove _cancelledForOff, it blocks real card cancellation" — the stated
  mechanism does not hold: a stale chargingOff cannot set the latch, because
  isStale() gates it first, so it cannot block the real removal that follows.
  The latch exists because charging == false persists for the rest of the
  process once seen, and _onEngineState fires ~1 Hz on live HR — without it we
  would issue a platform cancel() every second forever. Its failure mode after a
  restart is one REDUNDANT cancel (a no-op), never a missed one; "charging off
  clears the card even on a fresh process" already covers that.

Tests 1133 -> 1134; the new one verified to fail without the fix.
@abdulsaheel

Copy link
Copy Markdown
Collaborator Author

Second round reviewed. 1 of 4 real, fixed in a32e92f.

Fixed

Untimed announcement left a stale identity high-water (PR Agent) — real, and the failure direction is the bad one. A session announced with no usable strap clock has no identity of its own, but the previous high-water stayed in place and went on to judge a session it knew nothing about: the next usable timestamp falling at or below it got suppressed, costing the user a real "on the charger" confirmation. That's worse than the duplicate this PR exists to kill. Now cleared in memory and persisted as 0 (every real strap timestamp is far above it), so the untimed path is guarded only by the wall-clock cooldown — which is what ChargeAlertPolicy's untimed branch already assumes. Test seeds an old high-water, announces untimed, then announces a genuinely-new-but-lower-timestamped session; verified to fail without the fix.

Declined

"Low-armed not persisted on restore" and "untimed cooldown not restart-safe" — both scenarios require SharedPreferences writes to fail. Not a regression: before this PR _lowArmed was RAM-only and re-armed on every process start, so a dead store degrades to exactly the old behaviour while a working one is strictly better. There is no way to remember a latch without somewhere to remember it. I did note the asymmetry in a comment on _kLowArmed: charging keeps the recency window as a second line of defence, but a battery percentage carries no timestamp, so low-battery has only the key. _ThrowingStore is a fixture I wrote for robustness, not an observed production condition.

"Remove _cancelledForOff — it blocks real card cancellation" — the stated mechanism doesn't hold. The claim is that "a fresh process receiving a replayed stale chargingOff will call _notes.cancel … and set _cancelledForOff = true, which then blocks the cancel for the real removal". A stale chargingOff can't reach either statement — !ChargeAlertPolicy.isStale(...) gates the whole expression, which is the fix from round 1. Only a non-stale off sets the latch, and that is the real removal.

On the underlying question — should the latch be persisted? No: dropping it means calling cancel() on every update where charging == false, and state.charging stays false for the rest of the process once a chargingOff is seen, while _onEngineState fires at ~1 Hz on live HR. That's a platform round-trip every second, forever. Its failure mode after a restart is one redundant cancel — a no-op — never a missed one, i.e. the opposite of the claim. The existing "charging off clears the card even on a fresh process" test covers that path.

flutter analyze clean; 1134 tests.

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit a32e92f

Third review round.

A throwing sink aborted _apply before the writes below it, so a session could be
shown but not recorded and would re-announce after a restart. Reordering is not
the fix — persist-then-show just swaps which failure is fatal, letting a dead
store swallow the alert entirely. Neither step reads the other's result, so each
is now attempted independently and absorbs its own failure.

Also pinned the subtle two-latch interaction the reviewer flagged as fragile
rather than broken (it analysed it and concluded "no actual bug here", correctly):
a stale chargingOn re-opens the cancel latch, and the stale chargingOff that
follows must still not clear a card. That held, but nothing asserted it.

Tests 1134 -> 1135; both new assertions verified to fail without the change.
@abdulsaheel

Copy link
Copy Markdown
Collaborator Author

Third round. 1 of 2 real, fixed in 04dd2bf.

Presentation could take persistence down with it (PR Agent) — real. A throwing _notes.show aborted _apply before the _store.writeInt calls below it, so a session could be shown but never recorded, and would re-announce after a restart.

Worth noting the suggested reorder wouldn't have fixed it — persist-then-show just swaps which failure is fatal, letting a dead store swallow the alert entirely. Since neither step reads the other's result, each is now attempted independently and absorbs its own failure.

"Low-battery re-arm on stale chargingOn" — the analysis walks through it and reaches "No actual bug here", which matches what I get: a stale chargingOn re-opens _cancelledForOff, and the stale chargingOff that follows is still stopped by isStale. Agreed on the fragility point though — nothing asserted that interaction, so it's now pinned in "a stale on/off pair does not swallow the next genuine plug-in".

flutter analyze clean; 1135 tests. Both new assertions verified to fail without the change.


Heads-up on the check status: CodeRabbit's green tick on the last two pushes reads "Review rate limited" — it never analysed those commits, so its pass carries no signal here. Every finding actioned in rounds 2 and 3 came from PR Agent. Worth a re-run before merge if you want a second opinion on the latest diff.

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 04dd2bf

@abdulsaheel

Copy link
Copy Markdown
Collaborator Author

Fourth round — no action; this re-raises a finding the same reviewer resolved last round, and the new mechanism doesn't hold.

The claim is that a stale chargingOff "is NOT stale by the isStale check because its ts is also old" and will therefore cancel the card. That's self-contradictory — an old timestamp is exactly what isStale returns true for:

static bool isStale(int? tsEpoch, {required int wallNowSec}) =>
    timestampUsable(tsEpoch, wallNowSec: wallNowSec) &&
    wallNowSec - tsEpoch! > liveWindowSec;

The behaviour is asserted, not argued — a stale on/off pair does not swallow the next genuine plug-in feeds on(9h old) then off(8h old) and checks sink.cancelled does not contain idCharging. Added in 04dd2bf, passing.

The second half asks for coverage of the low-battery re-arm side, which already exists as a replayed plug-in does not re-arm the low alert — cited in the previous round's own analysis. Both pass:

DeviceAlerts charging a stale on/off pair does not swallow the next genuine plug-in  ✓
DeviceAlerts low battery a replayed plug-in does not re-arm the low alert            ✓

Calling the automated review done here: rounds 2–4 have converged on re-litigating settled ground, and CodeRabbit has been rate-limited (not actually analysing) for the last three pushes. Six real findings were fixed across the three rounds; the rest are documented above with reasons. Ready for human review.

@abdulsaheel
abdulsaheel merged commit b2a9812 into main Aug 3, 2026
3 checks passed
@abdulsaheel
abdulsaheel deleted the fix/charging-notification-replay branch August 3, 2026 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Android App: Repeated notification "Your band is on the charger" well after the charger has been removed

1 participant