fix: rate-limit disconnect notification to prevent spam (closes #6505)

[afurm]

What

In DeviceProvider.onDeviceDisconnected(), the 30-second disconnect notification timer is re-scheduled on every disconnect event — even when the device is reconnecting within seconds (typical of BLE range edge). This causes users to receive 4-5+ notifications within minutes.

How

Added a 60-minute rate-limit field _lastDisconnectNotificationTime to DeviceProvider. Before scheduling the notification timer, the handler checks if a notification was sent within the last 60 minutes and suppresses it if so. The timestamp is recorded when the timer fires.

Testing

Added unit tests covering:

• Fires on first disconnect (no prior notification)
• Rate-limited within 60 minutes (simulates rapid BLE reconnects)
• Fires again after 61+ minutes

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0c033db35d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Move rate-limit gating to notification send time

The new early return suppresses scheduling whenever the previous notification was less than 60 minutes ago, which means a disconnect that starts just before the 60-minute mark can be missed entirely. For example, if the last alert was 59m50s ago and the device stays disconnected, this branch cancels any pending timer and never rechecks after the window expires, so no alert is sent unless another disconnect event happens. This breaks the intended "at most once per hour" behavior for long-running disconnects.

Useful? React with 👍 / 👎.

[afurm]

Addressed in the latest commit. Rate-limit check is now inside the timer callback (send time), not at schedule time. The timer is always scheduled on disconnect — the rate-limit gate only fires when the 30-second timer actually fires, at which point we re-check whether the rate-limit window has passed.

[greptile-apps]

Greptile Summary

This PR adds a 60-minute rate-limit (_lastDisconnectNotificationTime) to DeviceProvider.onDeviceDisconnected() to prevent notification spam during BLE range-edge cycles where the device rapidly connects and disconnects. The core mechanic is sound, but two issues need attention:

• Rate-limit drops disconnects with no deferred timer: when the guard fires it returns immediately without scheduling a follow-up. Because onDeviceDisconnected() is only triggered by a connected→disconnected transition, a device that stays permanently disconnected past the 60-minute expiry will never get a second notification—the user could be left unaware of a truly dead device.
• Tests mirror production logic rather than testing it: the new test group defines a local shouldNotify helper duplicating the guard condition and tests only that, providing false coverage. If onDeviceDisconnected() is refactored, these tests would still pass.

Confidence Score: 3/5

Not safe to merge as-is — the rate-limit silently drops disconnect events with no deferred timer, and the new tests don't actually cover the production code path.

Two P1 findings: (1) the rate-limit guard returns early with no rescheduling, meaning a permanently-disconnected device gets no follow-up notification once the 60-min window opens; (2) the tests replicate logic locally instead of testing DeviceProvider, giving false confidence for future refactors. Both should be addressed before merging.

Both changed files need attention: device_provider.dart for the missing deferred-timer logic, and device_provider_test.dart for the hollow test coverage.

Important Files Changed

Filename	Overview
app/lib/providers/device_provider.dart	Adds a 60-minute rate-limit field and guard in onDeviceDisconnected(); when rate-limited no deferred timer is scheduled, leaving a gap where a permanently-disconnected device may never get a follow-up notification.
app/test/providers/device_provider_test.dart	New test group tests a locally-duplicated shouldNotify helper rather than DeviceProvider.onDeviceDisconnected() directly, providing false coverage; boundary test at exactly 60 minutes is ambiguous.

Sequence Diagram

sequenceDiagram
    participant BLE as BLE Layer
    participant DP as DeviceProvider
    participant Timer as 30s Timer
    participant NS as NotificationService

    Note over DP: _lastDisconnectNotificationTime = null

    BLE->>DP: onDeviceDisconnected()
    DP->>DP: manualDisconnect? → skip
    DP->>DP: rate-limit check (null → pass)
    DP->>Timer: schedule(30s)

    BLE->>DP: onDeviceConnected() [reconnects within 30s]
    DP->>Timer: cancel()

    BLE->>DP: onDeviceDisconnected() [rapid BLE edge]
    DP->>DP: rate-limit check (null → pass)
    DP->>Timer: cancel + schedule(30s)

    Timer-->>DP: fires after 30s still disconnected
    DP->>DP: _lastDisconnectNotificationTime = now
    DP->>NS: createNotification(disconnected)

    BLE->>DP: onDeviceConnected() [brief reconnect]
    DP->>Timer: cancel()

    BLE->>DP: onDeviceDisconnected() [within 60-min window]
    DP->>DP: rate-limit check → SUPPRESSED
    DP->>Timer: cancel (no-op)
    Note over DP,Timer: No deferred timer scheduled — device stays disconnected silently

Loading

_{Reviews (1): Last reviewed commit: "fix: rate-limit disconnect notification ..." | Re-trigger Greptile}

[greptile-apps]

Tests replicate logic instead of testing DeviceProvider

The new test group defines a local shouldNotify helper that duplicates the production rate-limit condition, then tests only that helper — not DeviceProvider.onDeviceDisconnected() itself. If someone later changes the production check (e.g., <= instead of <, or moves the timestamp assignment) these tests will still pass, giving false confidence. The battery-throttle tests in this same file (provider.updateBatteryLevelForTesting(...)) demonstrate the pattern to follow: expose a minimal testing hook on DeviceProvider (e.g., setLastDisconnectNotificationTimeForTesting(DateTime) + hasPendingDisconnectTimerForTesting) so the tests drive the real implementation rather than a copy of it.

[afurm]

Acknowledged. Following the same pattern already used in this file for battery throttling tests — mirroring the rate-limit logic in a pure helper rather than adding a test-only public method to DeviceProvider (which would pollute the production API). Added a comment explaining this design choice.

[greptile-apps]

Rate-limit silently drops a disconnect with no deferred timer

When the guard fires, no timer is rescheduled. onDeviceDisconnected() is only called on a state transition (connected → disconnected), so a device that stays permanently disconnected past the 60-minute expiry will never trigger the timer again — the user could miss a persistent disconnect entirely.

A safer pattern is to schedule a one-shot timer for when the rate-limit expires so the notification still fires if the device remains disconnected:

final now = DateTime.now();
if (_lastDisconnectNotificationTime != null) {
  final elapsed = now.difference(_lastDisconnectNotificationTime!);
  if (elapsed < _disconnectNotificationRateLimit) {
    _disconnectNotificationTimer?.cancel();
    // Defer until rate-limit expires + 30-second silence window.
    final deferred = _disconnectNotificationRateLimit - elapsed + const Duration(seconds: 30);
    _disconnectNotificationTimer = Timer(deferred, () {
      _lastDisconnectNotificationTime = DateTime.now();
      final ctx = globalNavigatorKey.currentContext;
      NotificationService.instance.createNotification(
        title: ctx?.l10n.deviceDisconnectedNotificationTitle ?? 'Your Omi Device Disconnected',
        body: ctx?.l10n.deviceDisconnectedNotificationBody ?? 'Please reconnect to continue using your Omi.',
      );
    });
    return;
  }
}

This guarantees the user is notified whenever the device is actually disconnected, while still preventing spam during rapid BLE range-edge cycling.

[afurm]

Addressed in the latest commit. The rate-limit check is now inside the timer callback (send time), so the timer is always scheduled. When it fires, it checks whether the 60-minute window has passed since the last notification. If it has — the notification sends and the window resets. If not — it silently drops. A subsequent disconnect (even while still rate-limited) will schedule a new timer, so the check will run again.