feat(notifications): cleaning-started ntfy + fix done summary race#2
Conversation
Adds a configurable "cleaning started" notification (ntfyOnStart, default on) and fixes the bug where the "cleaning done" notification on production was arriving without the stats summary. The "done" race: NotificationManager observed the cleaning -> idle UI transition before CleaningHistory::stopCollection's async getCharger callback finalized lastCleanStats. Result: stats.valid was false, or worse, stale data from the previous session leaked through (the discard path never invalidated lastCleanStats). Fix: bumped sessionId counter in both stop paths (success + discard), notification defers send via donePending until sessionId increments or a 5s timeout elapses. The start trigger fires on idle/non-cleaning -> CLEANINGRUNNING and explicitly excludes resumes from CLEANINGPAUSED, CLEANINGSUSPENDED, and DOCKING so pause/recharge resumes don't double-fire. Frontend mirrors the new ntfyOnStart Settings field end-to-end (types, defaults, form state, settings UI, mock server). Toggle is placed before "Cleaning done" to follow the cleaning lifecycle. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
npm version drift — drops libc fields from optional platform-specific deps. No runtime impact. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds a new ntfy “Cleaning started” notification toggle (default on) across firmware + frontend, and reworks firmware “Cleaning done” notifications to avoid a race with asynchronous end-of-session stat finalization.
Changes:
- Firmware: add
ntfyOnStartsetting and send a “Cleaning started” ntfy on fresh idle → cleaning transitions. - Firmware: defer “Cleaning done” notification until
CleaningHistorysession stats finalize (or timeout), using asessionIdmechanism. - Frontend: mirror the new
ntfyOnStartsetting end-to-end (types, defaults, mock server, settings UI).
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/views/settings/use-settings-form.ts | Adds ntfyOnStart to form state, dirty-check, and PATCH building. |
| frontend/src/views/settings/constants.ts | Adds ntfyOnStart default in DEFAULT_SERVER. |
| frontend/src/views/settings.tsx | Adds the “Cleaning started” toggle to the Notifications section. |
| frontend/src/types.ts | Extends SettingsData with ntfyOnStart. |
| frontend/mock/server.js | Mirrors ntfyOnStart in mock state and update whitelist. |
| frontend/package-lock.json | Lockfile normalization (removes some libc entries). |
| firmware/src/settings_manager.h | Adds Settings::ntfyOnStart field. |
| firmware/src/settings_manager.cpp | Persists/loads/applies/serializes ntfyOnStart. |
| firmware/src/config.h | Adds NVS key NVS_KEY_NTFY_ON_START. |
| firmware/src/notification_manager.h | Adds pending “done” state fields + helper methods. |
| firmware/src/notification_manager.cpp | Adds “started” notification and defers “done” send via donePending + timeout. |
| firmware/src/cleaning_history.h | Adds LastCleanStats.sessionId + sessionCounter. |
| firmware/src/cleaning_history.cpp | Increments sessionId on success + discard to support “done” deferral. |
Files not reviewed (1)
- frontend/package-lock.json: Language not supported
Comments suppressed due to low confidence (1)
firmware/src/notification_manager.cpp:39
tick()callsflushPendingDone()before checkingntfyEnabled,ntfyTopic, and WiFi connectivity. SincesendNotification()doesn’t re-check those gates, a pending "done" notification can still be sent even after the user disables notifications/clears the topic or WiFi drops. Consider gatingflushPendingDone()behind the same conditions (or droppingdonePendingwithout sending when notifications are disabled/topic empty/WiFi disconnected).
void NotificationManager::tick() {
// Drain a pending "done" notification independently of the fetch path —
// it's waiting on CleaningHistory's async stop, not our own state fetch.
if (donePending)
flushPendingDone();
if (fetchPending)
return;
// Skip if notifications disabled, no topic configured, or WiFi not connected
const Settings& s = settings.get();
if (!s.ntfyEnabled || s.ntfyTopic.isEmpty() || WiFi.status() != WL_CONNECTED)
return;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if ((wasCleaning || dockingDone || suspendedDone) && isIdle && cfg.ntfyOnDone && !donePending) { | ||
| // Defer the send: CleaningHistory::stopCollection finalizes stats | ||
| // inside an async getCharger callback, so reading getLastCleanStats() | ||
| // here can race and pull stale data from the prior session. Capture | ||
| // the current sessionId; flushPendingDone() fires once it increments | ||
| // (or after NTFY_DONE_PENDING_TIMEOUT_MS). | ||
| donePending = true; | ||
| doneTriggerSessionId = history.getLastCleanStats().sessionId; | ||
| donePendingSinceMs = millis(); | ||
| doneHostname = hostname; |
There was a problem hiding this comment.
The deferred "done" flow relies on flushPendingDone() being called again within NTFY_DONE_PENDING_TIMEOUT_MS (5s), but after the UI becomes idle this same tick sets the LoopTask interval to NOTIF_INTERVAL_IDLE_MS (30s). That means the pending notification may not flush for ~30s (and the 5s timeout is ineffective), causing delayed or bare "done" notifications. Consider temporarily forcing a short interval while donePending is true (or using a separate fast ticker) and restoring the normal active/idle interval once the pending notification is flushed.
| void NotificationManager::flushPendingDone() { | ||
| const LastCleanStats& stats = history.getLastCleanStats(); | ||
| bool finalized = stats.sessionId != doneTriggerSessionId; | ||
| bool timedOut = (millis() - donePendingSinceMs) >= NTFY_DONE_PENDING_TIMEOUT_MS; | ||
| if (!finalized && !timedOut) | ||
| return; | ||
|
|
||
| // If stopCollection finalized after we triggered, sessionId moved and stats | ||
| // are fresh; if we timed out, fire bare without stats rather than stale. | ||
| sendDoneNotification(doneTopic, doneHostname, finalized && stats.valid); | ||
| donePending = false; |
There was a problem hiding this comment.
flushPendingDone() assumes the just-finished session is finalized when stats.sessionId != doneTriggerSessionId. However, CleaningHistory::stopCollection() calls neato.getCharger(), and AsyncCache::get() can invoke callbacks synchronously on cache hits—so lastCleanStats.sessionId may already have been incremented before doneTriggerSessionId is captured. In that case finalized never becomes true and the code will eventually time out and send a bare notification even though fresh stats exist. Consider tracking the last observed sessionId in NotificationManager and, at trigger time, treating "already advanced since last poll" as finalized (send immediately with stats), otherwise wait for the increment.
Summary
ntfyOnStart(default on) — sends a ntfy when a cleaning cycle begins.Why the "done" stats were missing
Race in
notification_manager.cppvscleaning_history.cpp:NotificationManager::checkTransitionsobserves the cleaning → idle UI transition every poll cycle.CleaningHistory::stopCollectionfinalizeslastCleanStatsonly inside an asyncgetChargercallback (so it can record the end-of-session battery).stats.validis false → no summary. Worse, the discard path never invalidated stats, so a short session could leak the previous run's numbers.Fix: bumped a
sessionIdcounter in both stop paths (success + discard); notification now defers send viadonePendinguntil sessionId increments or 5s timeout (bare-body fallback).What ships
Firmware
cleaning_history.{h,cpp}—LastCleanStats.sessionId,sessionCounter, bumped in success + discard paths.notification_manager.{h,cpp}—donePendingstate machine,flushPendingDone(), new "started" trigger gated to non-cleaning → CLEANINGRUNNING (excludes pause/recharge resume).settings_manager.{h,cpp},config.h—ntfyOnStartfield, NVS key, full serdes.Frontend
types.ts,views/settings/{constants,use-settings-form}.ts,views/settings.tsx,mock/server.js— full mirror, toggle placed before "Cleaning done".Test plan
pio run -e c3-debug(RAM 17%, Flash 83.8%),pio check(no new findings),npm run check,npm run build.neato.int.weill-duflos.frand verify in the field.🤖 Generated with Claude Code