fix: close data-loss, privacy and absent-vs-zero defects across the app - #158
Conversation
An audit of the whole tree surfaced defects clustered in three seams the 886-test suite never reached: failure paths, absent-vs-zero coercion, and guards modelled with regexes and blocklists. Every fix below carries a regression test that fails against the old code. BLE — the safe-trim invariant held by ordering but not by errors DrainController.commit() cleared its buffer, then swallowed the durable commit's exception and returned void; the caller ACKed regardless, so the band trimmed flash for records that were never persisted. db.dart's own comment documents the OOM that makes this reachable (Crashlytics 0.9.13). commit() now reports failure, re-buffers at the front, and the ACK is gated on a TrimAckPolicy verdict. The drain path also had no session guard — the only two async paths lacking the check four siblings make — so a stale drainer could ACK an old token onto a new link. discardOpenChunk now poisons its burst, so a straggler terminal can't trim discarded records. Also: a failed connect no longer holds the band claim forever (starving the headless drainer), link-down tears the session down instead of leaking five timers and four subscriptions per drop, a corrupt far-future RTC is refused, and the Android boot wake goes through the headless gate. Persistence — three ways a good day could be destroyed - _markDaySkipped REPLACEd a real day_result with a skip marker on one 90 s isolate timeout, finalized, unrecoverable once raw was pruned. - Re-analyzing a day whose raw is gone derived an empty bundle with endSec 0, so it always looked finalizable and NULLed every scalar. - The per-sweep baseline snapshot was mutated by appendScalars, so each finished day polluted the window the next one read. None of the three can now overwrite a real result. Also: exportDaysDb passed onCreate with no version, so it threw ArgumentError on every call and had never worked; two unguarded ALTER TABLE ADD COLUMN collided with modernized CREATE helpers and bricked upgrades from old schemas permanently; the orphan guard missed the counter-PK eviction and decodedRrByCounterRange returned zero rows across a counter reset; deleteDays never cascaded workout_route; and local days were assumed to be exactly 86400 s. Privacy The coach SQL guard modelled table references with a regex that ignored comma cross-joins, and its blocklist was missing five real tables, so `SELECT r.lat, r.lng FROM v_sessions s, workout_route r` returned GPS coordinates into an LLM prompt. Replaced with a lexer + allow-list parser plus a structural gate that derives the permitted btree root pages by EXPLAINing the coach views, so a table added later is closed by default. Firebase collected by default: no collection flags existed in the plist or manifest, and the only gate ran after an async prefs read, so a fresh install auto-logged sessions and uploaded startup crashes before consent was read. Collection is now off at the platform level and enabled only after consent resolves. Absent is not zero "Wear stretches" read _n(segments) on a List and rendered 0 every day on every device. TrendBoard discarded the `has` flag the repository emits to mark absent days, so unworn days drew real bars — and the oxygen board printed a literal "0" for an unmeasured night. getDayWear collapsed "never measured" into worn_min 0, so an imported day asserted "not worn". Charts now take nullable values and render gaps as gaps. DeltaChip was also being fed an absolute delta and rendering it as a percentage, so sleeping 20 minutes more than last week displayed "20.0%". State Unpair could be silently undone by a late engine-state callback re-creating the pairing from a stale serial. The once-per-day notification guard was consumed before the notification was presented, so a recovery alert suppressed by quiet hours never fired and never retried. A denied notification permission was cached process-wide with no invalidation, and schedules added absolute Durations to TZDateTime and drifted across DST. kAlgoVersion 47 -> 48. The sibling analytics fixes from the same sweep are deliberately NOT claimed in that changelog: pubspec.yaml still pins the previous analytics SHA, and citing changes a pinned SHA does not contain is how v43 documented a readiness fix that stayed broken for three releases. The pin must be updated, and the version bumped again, once the analytics change merges.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR updates BLE synchronization, derivation, SQLite storage, imports, notifications, telemetry consent, DST handling, and UI rendering. It adds regression coverage and replaces fabricated defaults with explicit absent-value semantics. ChangesApplication correctness and safety
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Reviewer Guide 🔍(Review updated until commit b014c52)Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Latest suggestions up to b014c52 Explore these optional code suggestions:
Previous suggestionsSuggestions up to commit 9f53dd9
Suggestions up to commit b5c3352
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/ui/screens/detail_cards.dart (1)
1659-1702: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winOxygen strip drops missing nights instead of leaving a gap, contradicting the new methodNote.
presentfilters out every bucket withouthas == true, andvalues/labelsare built only frompresent— so a missing night is removed entirely rather than rendered as a gap. But the newmethodNotetext (added in this same change) states "Nights with no imported value are left blank." That isn't what the code does: the chart silently shows fewer bars for that week with no visual indication a night is missing, the same absent-as-zero-adjacent compaction issue this PR fixes elsewhere forRecapCard/MiniBars(day-shift from stripping nulls).Build
values/labelsfrom the fullbucketslist (nullingvaluewhenhas != true) and pass the nullable list straight intoLabeledBars, consistent withCoachChart's fix.🐛 Proposed fix to preserve gaps
- final present = buckets.where((b) => b['has'] == true).toList(); - if (present.isEmpty) { + final present = buckets.where((b) => b['has'] == true).toList(); + if (present.isEmpty) { return const _QuietState( icon: OsIcon.hydration, title: 'No recent oxygen trend yet', message: 'A few more nights of wear build the 7-night pattern.', ); } - final values = [ - for (final b in present) ((b['value'] as num?)?.toDouble() ?? 0), - ]; - final labels = [ - for (final b in present) _label((b['t_start'] as num?)?.toInt() ?? 0), - ]; + final values = [ + for (final b in buckets) + b['has'] == true ? (b['value'] as num?)?.toDouble() : null, + ]; + final labels = [ + for (final b in buckets) _label((b['t_start'] as num?)?.toInt() ?? 0), + ];🤖 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/screens/detail_cards.dart` around lines 1659 - 1702, Update the oxygen trend chart logic around `buckets`, `values`, and `labels` to preserve every bucket, including nights where `has != true`, instead of filtering through `present`. Build nullable values from the full `buckets` list, use the corresponding full labels, and pass that nullable list directly to `LabeledBars` so missing nights render as gaps while retaining the existing empty-state behavior when no bucket has data.
🧹 Nitpick comments (25)
lib/coach/coach_db.dart (3)
328-383: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
_clauseKeywordsis incomplete; unlisted keywords get swallowed as an alias.
when,then,else,end,is,in,not,like,between,exists,asc,desc,all,distinct,lateralare absent, so the bare-alias branch at Line 368 consumes them. This only produces false rejections of otherwise valid coach queries (fail-closed), but it will confuse the model. Worth extending the set.🤖 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/coach/coach_db.dart` around lines 328 - 383, The bare-alias check in _tableRefs must not consume SQL keywords as aliases. Extend _clauseKeywords with when, then, else, end, is, in, not, like, between, exists, asc, desc, all, distinct, and lateral, preserving existing alias handling for actual identifiers.
465-496: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
'Query failed: $e'forwards raw SQLite exception text into the LLM prompt.The exception message can embed statement text and, for constraint/type errors, column values. It is returned to the model and shipped to the third-party endpoint. Consider a fixed message and logging the detail locally.
🤖 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/coach/coach_db.dart` around lines 465 - 496, Update the catch-all error handling in runCoachSql to stop returning the raw exception text in the JSON response. Log the detailed exception locally using the existing logging mechanism, then return a fixed generic query-failure message to the caller while preserving the current SqlGuardError handling.
424-435: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winReject non-main cursors in
_btreeRoots(lib/coach/coach_db.dart:424-435)
OpenRead/ReopenIdxuseP2for the root page andP3for the database index (0 = main). Checking onlyP2lets a temp/attached cursor alias an allowed main root number, so this gate should reject non-main cursors before adding roots.🤖 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/coach/coach_db.dart` around lines 424 - 435, The _btreeRoots method must only collect roots for cursors targeting the main database. For OpenRead, OpenWrite, and ReopenIdx rows, inspect the EXPLAIN P3 value and skip entries whose database index is nonzero before evaluating or adding P2 roots; preserve the existing positive-root filtering.test/coach_sql_structural_test.dart (1)
32-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSeeding inside a
test()makes the rest of the file order- and filter-dependent.
runCoachSql still serves the allowed viewsassertsrow_count == 1, which only holds if this test ran first. Running a single test with--plain-name(or any future reordering) breaks it. Move the inserts intosetUpAlland keep only the assertion here.🤖 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/coach_sql_structural_test.dart` around lines 32 - 56, Move the metric_series, sessions, and route-point setup from the test named “seeds a session + a route + derived scalars” into the file’s setUpAll callback, ensuring it runs before the coach SQL assertions regardless of test order. Leave this test focused only on verifying the workout_route count, and preserve the existing seed values and assertion.lib/coach/coach_engine.dart (1)
219-237: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
_trimHistorycan silently drop the turn the user just asked about.Once everything older is gone, the loop keeps removing until
_history.length == 1, so a session that is already at the ceiling can discard the current user message beforepostChatever sees it, and the model answers with no question. SincepostChatalready fails closed on oversize, consider stopping the trim at the newest user turn and letting the request ceiling surface the actionable error.🤖 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/coach/coach_engine.dart` around lines 219 - 237, Update _trimHistory so trimming never removes the newest user turn: stop dropping history once the oldest remaining entry is the current user message, even if the history still exceeds kMaxHistoryChars. Let postChat’s existing oversize-request handling surface the error instead of sending a request without the user’s question.lib/notify/notification_center.dart (1)
138-151: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueConsider
prefs.reload()before reading the day guard.
NotificationIds._slotForreloads for exactly this reason: derivation also runs in the WorkManager isolate, whoseSharedPreferencescache won't see a guard written by the other isolate. Worst case here is a redundantemitthatFiredKeyStore.claimabsorbs, so this is cheap insurance rather than a live defect.♻️ Proposed refactor
SharedPreferences? prefs; try { prefs = await SharedPreferences.getInstance(); + await prefs.reload(); // the OTHER isolate may have consumed the guard } catch (_) { prefs = null; // no store — [emit]'s own FiredKeyStore still dedupes }🤖 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/notification_center.dart` around lines 138 - 151, Update the SharedPreferences guard flow around the local prefs variable to call prefs.reload() before reading prefsKey, preserving the existing failure handling and emit/FiredKeyStore behavior if reload fails.lib/state/app_state.dart (1)
2322-2338: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: hoist
'alarm_epoch'into a named constant.The literal now appears in
_init,setAlarm,disableAlarmand here; every other persisted key in this class already uses astatic const String _kX.🤖 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/state/app_state.dart` around lines 2322 - 2338, Define a static const String for the persisted alarm epoch key alongside the other key constants in the app-state class, then replace the repeated 'alarm_epoch' literals in _init, setAlarm, disableAlarm, and _clearArmedAlarmState with that constant.test/ble_safe_trim_test.dart (1)
92-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
d2doesn't proved's records survived — re-commit onditself.The comment claims this proves the re-buffered records are still there, but
d2is a fresh controller that never had a failing commit; it only shows the happy path. Committing again ondwould actually assert the restored buffer's contents.♻️ Suggested: assert against the controller that failed
- // Proof they are really still there: a later successful commit ships - // every record AND the archived one. - final seenRaws = <String>[]; - final seenArchives = <String>[]; - final d2 = _drainWith((raws, samples, token, {archives}) async { - seenRaws.addAll(raws.map((r) => r.hex)); - seenArchives.addAll((archives ?? const []).map((a) => a.hex)); - }); - // (rebuild the same state on a controller whose commit succeeds) - d2.onHistoricalRecord(_raw(1), _sample(1)); - d2.onHistoricalRecord(_raw(2), _sample(2)); - d2.onUndecodableRecord(_archive(3)); - expect(await d2.commit(_tokenA), isTrue); - expect(seenRaws, [_hex(1), _hex(2)]); - expect(seenArchives, [_hex(3)]); + // Proof they are really still there: the NEXT commit on the SAME + // controller ships every re-buffered record AND the archived one. + final seenRaws = <String>[]; + final seenArchives = <String>[]; + shouldFail = false; + onCommitSpy = (raws, samples, token, {archives}) async { + seenRaws.addAll(raws.map((r) => r.hex)); + seenArchives.addAll((archives ?? const []).map((a) => a.hex)); + }; + expect(await d.commit(_tokenA), isTrue); + expect(seenRaws, [_hex(1), _hex(2)]); + expect(seenArchives, [_hex(3)]);(build
dwith a mutableshouldFailflag + swappable sink, as the ordering test at Line 109 already does)🤖 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/ble_safe_trim_test.dart` around lines 92 - 107, Update the failing-commit test to verify the restored records on the original controller d rather than a fresh d2. Use a mutable shouldFail flag and swappable sink as in the nearby ordering test, make d’s first commit fail, then switch the sink to capture output and commit d again, asserting that both buffered raw records and the archived record are emitted.lib/ble/ble_engine.dart (1)
1973-1980: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRejecting the read also drops the bounded SET_CLOCK correction for exactly the band that needs it.
Before this gate, a far-future
clock_epochfell intoshouldSetClock(drift > 86400) and got up to 3 bounded SET_CLOCK re-issues. Now the reject branch only counts and logs, so a band whose RTC really is stuck far-future never gets a corrective re-issue for the rest of the session — the periodic re-verify just re-reads and lands here again. Not correlating is right; not correcting looks unintended.♻️ Suggested: keep the bounded correction on the reject path
if (!ClockPolicy.acceptsClockRead(dev, wall)) { _corruptClockReadCount++; _log( '[SYNC] GET_CLOCK clock_epoch=$dev is implausibly far in the future ' '— treating as a corrupt strap RTC read; NOT correlating the strap ' 'clock (alarms fall back to the raw wall epoch) ' '(corrupt_clock_reads_total=$_corruptClockReadCount).', ); + // The read is untrustworthy for CORRELATION, but a far-future RTC is + // still an RTC that needs setting. Re-issue under the same bounded + // budget so a genuinely-wrong clock can still be corrected. + if (_clockCorrectTries < 3) { + _clockCorrectTries++; + _log('Corrupt RTC read — re-issuing SET_CLOCK ' + '(attempt $_clockCorrectTries/3).'); + unawaited(setClock()); + } } else {🤖 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/ble/ble_engine.dart` around lines 1973 - 1980, Update the ClockPolicy.acceptsClockRead rejection branch in the clock-read handling flow to retain the existing bounded SET_CLOCK correction behavior for implausibly future RTC values, while still excluding the read from strap-clock correlation. Reuse the existing shouldSetClock correction and retry-limit mechanisms so corrections remain capped at three re-issues and subsequent periodic verification preserves the current logging and fallback behavior.test/ble_transport_guards_test.dart (1)
38-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test can pass vacuously — assert the precondition.
The only assertion is that no log line contains
yielding, which is also satisfied if the drainer never reached_claimBandat all (the test depends on how flutter_blue_plus fails on the host). Pinning the state the scenario is actually about makes it non-vacuous.🧪 Suggested: assert the stale-claim precondition
await foreground.connectToRemoteId('AA:BB:CC:DD:EE:FF'); + // Precondition: the foreground connect failed and left no live link — + // this is the "stale claim" the drainer must be able to take. + expect(foreground.holdsBandLink, isFalse); final drainerLogs = <String>[]; @@ expect( drainerLogs.where((l) => l.contains('yielding')), isEmpty, reason: 'the drainer must actually attempt the band', ); + expect(drainer.holdsBandLink, isFalse); + expect(BleEngine.bandClaimed, isFalse, + reason: 'the drainer\'s own failed connect must release too');🤖 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/ble_transport_guards_test.dart` around lines 38 - 66, Make the test non-vacuous by asserting, after the foreground connection attempt, that the expected stale foreground claim/precondition exists before creating the background drainer. Use the relevant observable claim state or established test hook used by BleEngine, then retain the existing yielding-log assertion to verify the drainer proceeds.lib/compute/crossday_pipeline.dart (1)
393-419: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNormalize the map key through
_dateKeyso lookup can't silently miss.
byDateis keyed by the rawdates[i]string, but lookup at Line 416 uses_dateKey(cursor)(YYYY-MM-DD). Any accepted-but-differently-formatted date (2026-03-08T00:00:00Z, or a+00:00suffix) parses fine, gets stored under the raw string, and then never matches — the whole series degrades to zeros with no error, which is exactly the CTL/ATL/TSB corruption this change is fixing.♻️ Key both sides through the same formatter
for (var i = 0; i < dates.length && i < trimps.length; i++) { final date = dates[i]; - if (date.isEmpty || DateTime.tryParse(date) == null) continue; - byDate[date] = trimps[i] ?? 0.0; - first ??= date; - if (last == null || date.compareTo(last) > 0) last = date; - if (date.compareTo(first) < 0) first = date; + final parsed = date.isEmpty ? null : DateTime.tryParse(date); + if (parsed == null) continue; + final key = _dateKey(parsed); + byDate[key] = trimps[i] ?? 0.0; + first ??= key; + if (last == null || key.compareTo(last) > 0) last = key; + if (key.compareTo(first) < 0) first = key; }🤖 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/compute/crossday_pipeline.dart` around lines 393 - 419, Normalize each valid date through _dateKey before storing it in byDate within _denseDailyTrimp, so its keys match the _dateKey(cursor) lookups. Preserve the existing parsing, duplicate handling, and densification behavior while ensuring ISO timestamps and timezone-suffixed dates map to their calendar day.test/day_window_dst_test.dart (2)
80-96: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
setUpAll/tearDownAllcall POSIX-only FFI unguarded while the skip lives on each test.Every test carries
skip: Platform.isWindows, but the fixture itself doesn't:_setProcessTzlooks upsetenv/tzsetviaDynamicLibrary.process(), which won't resolve on Windows. Today this is only safe because the framework skipssetUpAllwhen no test in the group runs — add one non-skipped test to this file and Windows breaks. Cheap to make explicit.♻️ Guard the fixture too
setUpAll(() async { + if (Platform.isWindows) return; _setProcessTz('America/New_York');tearDownAll(() async { await LocalDb.close(); await databaseFactory.deleteDatabase( p.join(await databaseFactory.getDatabasesPath(), LocalDb.dbName), ); - _setProcessTz(originalTz); + if (!Platform.isWindows) _setProcessTz(originalTz); });🤖 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/day_window_dst_test.dart` around lines 80 - 96, Guard the entire DST fixture setup and teardown in setUpAll and tearDownAll with the same Windows check used by the tests, so _setProcessTz and database initialization never run on Windows. Preserve the existing setup and cleanup behavior on supported platforms.
36-47: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
malloc.freefor both UTF-8 bufferstoNativeUtf8()defaults tomalloc, so freeingkeyandvaluewithmalloc.free(...)keeps the allocation/free pair explicit.🤖 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/day_window_dst_test.dart` around lines 36 - 47, Update the environment setup block around the `key` and `value` buffers to free both `toNativeUtf8()` allocations with `malloc.free(...)` instead of `calloc.free(...)`, including the `finally` cleanup for `key`.test/db_p0_fixes_test.dart (1)
224-232: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTests share one database and wipe tables mid-file, so they're order-coupled.
fix 11,fix 10,fix 8andfix 1each start by deleting whole tables (events,day_result,decoded_onehz,decoded_rr). The earlierfix 3/fix 4cases only pass because they're declared first. Moving the cleanup into asetUp(or scoping counters/day ids per test) makes the file order-independent.Also applies to: 319-325, 418-425
🤖 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/db_p0_fixes_test.dart` around lines 224 - 232, Make the database tests order-independent by moving full-table cleanup for events, day_result, decoded_onehz, and decoded_rr into shared setUp logic, or otherwise isolating each test’s data with unique counters/day IDs. Update the tests identified by fix 11, fix 10, fix 8, and fix 1 while preserving their existing assertions and setup behavior.test/telemetry_consent_default_test.dart (1)
30-36: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBoth config assertions are attribute/element-order sensitive.
plistValueAftertakes the first tag after the key (so a<string>false</string>form reads back as'string'), and the manifest regex requiresandroid:nameto precedeandroid:valuewith no intervening attributes. Either is a legitimate way to express the same config, and both would fail the guard rather than the thing it's guarding. Loosening the manifest match to allow either attribute order would make this less likely to break on an unrelated manifest edit.Also applies to: 62-70
🤖 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/telemetry_consent_default_test.dart` around lines 30 - 36, Update plistValueAfter to parse both self-closing values and paired element content, returning the actual value rather than the tag name. Relax the manifest assertion regex so android:name and android:value may appear in either order with intervening attributes, while preserving validation of the expected configuration.test/derive_result_protection_test.dart (2)
286-287: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest name contradicts itself.
"does not overwrite ... with a worse one — but is allowed to replace it" reads as two opposite claims; the body only asserts that a skip marker replaces another skip marker. Something like
'a skip marker may replace another skip marker (no user data to lose)'matches what is verified.🤖 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/derive_result_protection_test.dart` around lines 286 - 287, Rename the test case describing skip-marker replacement to remove the contradictory “does not overwrite” versus “is allowed to replace” wording. Use a name that clearly states a skip marker may replace another skip marker, matching the behavior asserted by the test body.
162-167: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConditional assertion can pass vacuously.
If
runDayswrites nothing,rowis null and the test asserts nothing. With the newproducedNothingpath a fresh day does get an unfinalized row, so this can be tightened toexpect(row, isNotNull)followed by thefinalized == 0check — otherwise a future regression that stops writing the row entirely would go unnoticed here.🤖 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/derive_result_protection_test.dart` around lines 162 - 167, Update the assertion around LocalDb.dayResult(freshDay) so the test first requires row to be non-null, then asserts row['finalized'] equals 0 without a conditional guard. Preserve the existing reason text and ensure a missing result causes the test to fail.test/derive_day_window_test.dart (1)
108-116: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThis assertion restates the implementation rather than testing it.
tzOffsetSecondsAt(t)is literallyDateTime.fromMillisecondsSinceEpoch(t * 1000, isUtc: false).timeZoneOffset.inSeconds, so this loop can never fail regardless of correctness. The jan/jul divergence check below (lines 119-124) is the part that carries signal. Consider dropping the tautology, or replacing it with a fixed-zone expectation guarded onDateTime.now().timeZoneNameso it asserts something the implementation could get wrong.🤖 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/derive_day_window_test.dart` around lines 108 - 116, Remove the tautological loop comparing tzOffsetSecondsAt(t) with the identical DateTime expression; retain the jan/jul divergence assertion below as the meaningful coverage for seasonal offset behavior.lib/compute/derivation_engine.dart (3)
1892-1895: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDangling dartdoc reference to the removed
appendScalars.
appendScalarsis gone, but_refreshBaselines' doc comment (around line 2376) still says the artifact is rebuilt "rather than persisting the in-memory cache the sweep mutated via[_BaselineHistoryCache.appendScalars]". That square-bracket reference no longer resolves and the paragraph now describes behavior that cannot happen. Worth updating in the same change so the comment doesn't outlive the bug it describes.🤖 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/compute/derivation_engine.dart` around lines 1892 - 1895, Update the _refreshBaselines documentation to remove the obsolete [_BaselineHistoryCache.appendScalars] reference and the outdated claim that the sweep mutates the in-memory cache. Keep the documentation aligned with the current frozen-snapshot and rebuilt-artifact behavior described near the sweep logic.
3745-3761: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate day-window math — reuse
data/day_label.dart.This PR adds
localDayStartSec/localDayEndSecinlib/data/day_label.dartwith the same DST-correctDateTime(y, m, d + 1)logic, plus strict label parsing._localDayLabelToSec/_localNextDayLabelToSechere duplicate it and degrade a malformed label to0instead ofnull, so the two implementations can drift. Delegating keeps one source for the local-day window concern.As per coding guidelines, "Maintain one source per concern" and "Use todayLabel() or dayLabelOf() from data/day_label.dart for local day labels".
Also applies to: 3786-3790
🤖 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/compute/derivation_engine.dart` around lines 3745 - 3761, Remove the duplicate _localDayLabelToSec and _localNextDayLabelToSec implementations from the derivation engine and reuse localDayStartSec and localDayEndSec from data/day_label.dart at their call sites. Preserve strict malformed-label handling by propagating the shared helpers’ null result rather than converting invalid labels to 0, and use the existing day-label utilities as the single source for local-day windows.Source: Coding guidelines
1111-1126: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
portleaks ifIsolate.spawnthrows.
ReceivePortis created before theawait Isolate.spawn(...); if spawn fails (e.g. OOM), thetry/finallybelow hasn't been entered yet, so the port is never closed and keeps the isolate's event loop referenced. Same shape applies in_runIsolateCancellable(line 3330) and_runDayBlocksCancellable.♻️ Guard the spawn
- final isolate = await Isolate.spawn( - derivationPrepareWorker, - port.sendPort, - onError: port.sendPort, - onExit: port.sendPort, - ); + final Isolate isolate; + try { + isolate = await Isolate.spawn( + derivationPrepareWorker, + port.sendPort, + onError: port.sendPort, + onExit: port.sendPort, + ); + } catch (_) { + port.close(); + rethrow; + }🤖 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/compute/derivation_engine.dart` around lines 1111 - 1126, Guard ReceivePort cleanup around Isolate.spawn in the affected derivation worker methods, including the flow containing derivationPrepareWorker, _runIsolateCancellable, and _runDayBlocksCancellable. Ensure each port is closed if spawning throws before the existing cleanup scope is entered, while preserving normal post-spawn handling and avoiding double-close issues.lib/import/whoop_import.dart (1)
123-144: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winTwo divergent "is this a real day result?" predicates.
DerivationEngine._isRealDayResult(lib/compute/derivation_engine.dartaround line 2024) guards the exact same destructiveputDayResultreplace, but uses different rules: it requires at least one non-null scalar and ignores theimportedmarker, while this one accepts any non-skipped decodable payload and treatsimported: trueas replaceable. The differing behavior looks intentional (the importer may replace a vendor snapshot; the engine may not blank one), but nothing in either place records that, so a future edit to one will silently desynchronize the protection.Worth extracting a single shared helper with an explicit
allowVendorSnapshotsflag, or at minimum cross-referencing the two doc comments.As per coding guidelines, "Maintain one source per concern".
🤖 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/import/whoop_import.dart` around lines 123 - 144, Unify the real-day protection logic used by _isRealDerivedDay and DerivationEngine._isRealDayResult into one shared helper with an explicit allowVendorSnapshots option, preserving each caller’s current behavior: the importer may replace imported vendor snapshots, while the derivation engine must retain them and require a non-null scalar. Update both doc comments to document this distinction and prevent future predicate drift.Source: Coding guidelines
test/crossday_pipeline_test.dart (1)
192-211: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueNit:
+ i * 86400shifts local clock time across a DST boundary.
onset/wakeare built from localDateTimes, then advanced by fixed 86 400 s steps over 2024-03-04 → 03-17 — which spans the US spring-forward. In a DST-observing zone the later nights land at 00:30/01:30 local instead of 23:30/00:30, so some of them stop crossing midnight and the test's premise ("the only segment of each night crosses midnight") weakens. Building each night withDateTime(2024, 3, 4 + i, 23, 30)keeps the wall-clock intent in every zone.🤖 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/crossday_pipeline_test.dart` around lines 192 - 211, Update the nights helper to construct each night's onset and wake timestamps from local DateTime values using the calendar day 4 + i, rather than adding fixed i * 86400 seconds to the initial epochs. Apply the same per-night timestamps to onset_sec, wake_sec, and hypnogram start/end so every entry preserves the intended 23:30-to-00:30 wall-clock crossing across DST.lib/compute/derive_prepare.dart (2)
188-197: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
localDayEndSecfromdata/day_label.dartinstead of a second next-midnight implementation.
lib/data/db.dart(_localDayWindow) andlib/data/local_repository_impl.dart(_localDayEndSec) both resolve "next local midnight" throughlocalDayEndSec(dayId). This adds a third implementation with slightly different input handling (DateTime.tryParseaccepts full ISO strings, not justYYYY-MM-DD), so the day-boundary semantics can drift between the derive path and the storage/UI paths.As per coding guidelines, "Maintain one source per concern" and "Use todayLabel() or dayLabelOf() from data/day_label.dart for local day labels".
♻️ Delegate to the shared day-window helper
-int localNextMidnightSecForDayLabel(String dayId) { - final d = DateTime.tryParse(dayId); - if (d == null) return 0; - return DateTime(d.year, d.month, d.day + 1).millisecondsSinceEpoch ~/ 1000; -} +int localNextMidnightSecForDayLabel(String dayId) => + localDayEndSec(dayId) ?? 0;🤖 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/compute/derive_prepare.dart` around lines 188 - 197, Remove the duplicate localNextMidnightSecForDayLabel implementation and reuse localDayEndSec from data/day_label.dart in the derive path. Ensure callers use the shared day-label semantics, converting inputs through todayLabel() or dayLabelOf() where needed rather than accepting arbitrary ISO strings.Source: Coding guidelines
365-370: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
_numrejects the exact case the docstring cites.The comment says an older/importing path "can hand back a String … where an INTEGER is declared", but
v is num ? v : nullmaps'72'to null — sorec_tsrows get skipped andhr/accel silently become 0 rather than being recovered. If string storage-class drift is real, parse it.♻️ Accept numeric strings too
- static num? _num(Object? v) => v is num ? v : null; + static num? _num(Object? v) => + v is num ? v : (v is String ? num.tryParse(v) : null);🤖 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/compute/derive_prepare.dart` around lines 365 - 370, Update the _num helper to preserve numeric values and also parse valid numeric strings, including integer and decimal representations; return null only for null or non-numeric inputs so rec_ts, hr, and acceleration values are recovered from SQLite storage-class drift.
🤖 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/coach/coach_db.dart`:
- Around line 390-418: The _allowedRootPages cache must be invalidated when the
SQLite schema changes. Update _allowedRootPages to read PRAGMA schema_version
through rawQuery and associate _allowedRoots with that version, recomputing
roots whenever the version differs; preserve the existing fast path when the
cached version still matches.
In `@lib/coach/coach_engine.dart`:
- Around line 212-213: Update the request-size enforcement around
kMaxRequestBytes to measure the UTF-8 encoded payload length rather than
payload.length, and use that same byte count for the user-facing KB value.
Preserve the existing limit and rejection behavior, and send the encoded bytes
via the request body or otherwise ensure the transmitted body uses the measured
UTF-8 representation.
In `@lib/data/day_label.dart`:
- Around line 26-37: Update _partsOf to validate that the parsed year, month,
and day form the exact requested calendar date, rejecting out-of-range values
such as invalid months or days instead of allowing DateTime normalization.
Preserve the existing null result for malformed labels and return the parsed
components only when they represent a valid date.
In `@lib/data/db.dart`:
- Around line 88-93: Update the app-database open call in _open() to pass the
shared schemaVersion constant instead of the literal 26, ensuring all database
opens in the class use the single schema-version source.
- Around line 2591-2605: Update the endpoint-resolution condition in the
query-selection logic to use the timestamp window whenever at least one endpoint
resolves, changing the requirement from n == want to n >= 1. Preserve the
existing bounds-based rawQuery and leave the normalized counter-range fallback
only for cases where no endpoint resolves.
- Around line 1417-1476: Update _migrateLegacyTable to accept and operate on the
ambient DatabaseExecutor when invoked from onUpgrade, avoiding a nested
db.transaction; retain an explicit transaction only for the onOpen repair path.
Preserve the existing migration, copy, skipCopy, and legacy-table cleanup
behavior while routing both paths through the shared logic.
In `@lib/import/noop_import.dart`:
- Line 200: Update AppState.importNoopCsv() and the import screen to propagate
and display the lateRows value from NoopImportResult, rather than returning or
consuming only days. Preserve the existing days and totalRows handling while
ensuring a non-zero late-row count reaches the user-facing UI.
In `@lib/import/whoop_import.dart`:
- Around line 255-258: Update the finalized assignment in the WHOOP import loop
to finalize a date only when it is older than the device’s raw-data retention
boundary, rather than when it is merely absent from the snapshotted rawDays set.
Reuse the existing lastDecodedRecTs() and raw-retention calculation used by
_pruneOldDecoded, ensuring dates still able to receive future raw data remain
unfinalized.
In `@lib/state/app_state.dart`:
- Around line 750-779: Update AppState.dispose() to tear down the owned
_routeTracker before super.dispose(), canceling or disposing it through the
RouteTracker lifecycle API and clearing the field if applicable. Ensure its GPS
subscription and buffered-point flush cannot continue writing after AppState
disposal, while preserving the existing cleanup order for the other owned
resources.
In `@lib/sync/headless_boot.dart`:
- Around line 113-124: Add an onError handler to the fire-and-forget Future
chain around runBootSyncThroughGate(lease) in the headless boot flow, ensuring
failures are handled rather than escaping as unhandled async errors. Preserve
the existing then callback and its logging behavior, and report the caught error
through the established debugging or logging mechanism.
- Around line 47-62: Update runBootSyncThroughGate to catch failures from
HeadlessSyncGate.tryRun, release the acquired BandLease, and rethrow the
original error. Also update the unawaited(...) chain at
lib/sync/headless_boot.dart lines 113-124 to handle errors with onError or
catchError and log them, preventing unhandled async failures; both locations
require changes.
In `@lib/telemetry/telemetry_service.dart`:
- Around line 46-50: Update the enabled setter in the telemetry service to
handle revocation immediately: when value is false, clear any unsent Crashlytics
reports, disable collection for the remainder of the process, and preserve the
restart-required behavior until the next cold start. Keep the existing enable
path and consent state updates intact.
---
Outside diff comments:
In `@lib/ui/screens/detail_cards.dart`:
- Around line 1659-1702: Update the oxygen trend chart logic around `buckets`,
`values`, and `labels` to preserve every bucket, including nights where `has !=
true`, instead of filtering through `present`. Build nullable values from the
full `buckets` list, use the corresponding full labels, and pass that nullable
list directly to `LabeledBars` so missing nights render as gaps while retaining
the existing empty-state behavior when no bucket has data.
---
Nitpick comments:
In `@lib/ble/ble_engine.dart`:
- Around line 1973-1980: Update the ClockPolicy.acceptsClockRead rejection
branch in the clock-read handling flow to retain the existing bounded SET_CLOCK
correction behavior for implausibly future RTC values, while still excluding the
read from strap-clock correlation. Reuse the existing shouldSetClock correction
and retry-limit mechanisms so corrections remain capped at three re-issues and
subsequent periodic verification preserves the current logging and fallback
behavior.
In `@lib/coach/coach_db.dart`:
- Around line 328-383: The bare-alias check in _tableRefs must not consume SQL
keywords as aliases. Extend _clauseKeywords with when, then, else, end, is, in,
not, like, between, exists, asc, desc, all, distinct, and lateral, preserving
existing alias handling for actual identifiers.
- Around line 465-496: Update the catch-all error handling in runCoachSql to
stop returning the raw exception text in the JSON response. Log the detailed
exception locally using the existing logging mechanism, then return a fixed
generic query-failure message to the caller while preserving the current
SqlGuardError handling.
- Around line 424-435: The _btreeRoots method must only collect roots for
cursors targeting the main database. For OpenRead, OpenWrite, and ReopenIdx
rows, inspect the EXPLAIN P3 value and skip entries whose database index is
nonzero before evaluating or adding P2 roots; preserve the existing
positive-root filtering.
In `@lib/coach/coach_engine.dart`:
- Around line 219-237: Update _trimHistory so trimming never removes the newest
user turn: stop dropping history once the oldest remaining entry is the current
user message, even if the history still exceeds kMaxHistoryChars. Let postChat’s
existing oversize-request handling surface the error instead of sending a
request without the user’s question.
In `@lib/compute/crossday_pipeline.dart`:
- Around line 393-419: Normalize each valid date through _dateKey before storing
it in byDate within _denseDailyTrimp, so its keys match the _dateKey(cursor)
lookups. Preserve the existing parsing, duplicate handling, and densification
behavior while ensuring ISO timestamps and timezone-suffixed dates map to their
calendar day.
In `@lib/compute/derivation_engine.dart`:
- Around line 1892-1895: Update the _refreshBaselines documentation to remove
the obsolete [_BaselineHistoryCache.appendScalars] reference and the outdated
claim that the sweep mutates the in-memory cache. Keep the documentation aligned
with the current frozen-snapshot and rebuilt-artifact behavior described near
the sweep logic.
- Around line 3745-3761: Remove the duplicate _localDayLabelToSec and
_localNextDayLabelToSec implementations from the derivation engine and reuse
localDayStartSec and localDayEndSec from data/day_label.dart at their call
sites. Preserve strict malformed-label handling by propagating the shared
helpers’ null result rather than converting invalid labels to 0, and use the
existing day-label utilities as the single source for local-day windows.
- Around line 1111-1126: Guard ReceivePort cleanup around Isolate.spawn in the
affected derivation worker methods, including the flow containing
derivationPrepareWorker, _runIsolateCancellable, and _runDayBlocksCancellable.
Ensure each port is closed if spawning throws before the existing cleanup scope
is entered, while preserving normal post-spawn handling and avoiding
double-close issues.
In `@lib/compute/derive_prepare.dart`:
- Around line 188-197: Remove the duplicate localNextMidnightSecForDayLabel
implementation and reuse localDayEndSec from data/day_label.dart in the derive
path. Ensure callers use the shared day-label semantics, converting inputs
through todayLabel() or dayLabelOf() where needed rather than accepting
arbitrary ISO strings.
- Around line 365-370: Update the _num helper to preserve numeric values and
also parse valid numeric strings, including integer and decimal representations;
return null only for null or non-numeric inputs so rec_ts, hr, and acceleration
values are recovered from SQLite storage-class drift.
In `@lib/import/whoop_import.dart`:
- Around line 123-144: Unify the real-day protection logic used by
_isRealDerivedDay and DerivationEngine._isRealDayResult into one shared helper
with an explicit allowVendorSnapshots option, preserving each caller’s current
behavior: the importer may replace imported vendor snapshots, while the
derivation engine must retain them and require a non-null scalar. Update both
doc comments to document this distinction and prevent future predicate drift.
In `@lib/notify/notification_center.dart`:
- Around line 138-151: Update the SharedPreferences guard flow around the local
prefs variable to call prefs.reload() before reading prefsKey, preserving the
existing failure handling and emit/FiredKeyStore behavior if reload fails.
In `@lib/state/app_state.dart`:
- Around line 2322-2338: Define a static const String for the persisted alarm
epoch key alongside the other key constants in the app-state class, then replace
the repeated 'alarm_epoch' literals in _init, setAlarm, disableAlarm, and
_clearArmedAlarmState with that constant.
In `@test/ble_safe_trim_test.dart`:
- Around line 92-107: Update the failing-commit test to verify the restored
records on the original controller d rather than a fresh d2. Use a mutable
shouldFail flag and swappable sink as in the nearby ordering test, make d’s
first commit fail, then switch the sink to capture output and commit d again,
asserting that both buffered raw records and the archived record are emitted.
In `@test/ble_transport_guards_test.dart`:
- Around line 38-66: Make the test non-vacuous by asserting, after the
foreground connection attempt, that the expected stale foreground
claim/precondition exists before creating the background drainer. Use the
relevant observable claim state or established test hook used by BleEngine, then
retain the existing yielding-log assertion to verify the drainer proceeds.
In `@test/coach_sql_structural_test.dart`:
- Around line 32-56: Move the metric_series, sessions, and route-point setup
from the test named “seeds a session + a route + derived scalars” into the
file’s setUpAll callback, ensuring it runs before the coach SQL assertions
regardless of test order. Leave this test focused only on verifying the
workout_route count, and preserve the existing seed values and assertion.
In `@test/crossday_pipeline_test.dart`:
- Around line 192-211: Update the nights helper to construct each night's onset
and wake timestamps from local DateTime values using the calendar day 4 + i,
rather than adding fixed i * 86400 seconds to the initial epochs. Apply the same
per-night timestamps to onset_sec, wake_sec, and hypnogram start/end so every
entry preserves the intended 23:30-to-00:30 wall-clock crossing across DST.
In `@test/day_window_dst_test.dart`:
- Around line 80-96: Guard the entire DST fixture setup and teardown in setUpAll
and tearDownAll with the same Windows check used by the tests, so _setProcessTz
and database initialization never run on Windows. Preserve the existing setup
and cleanup behavior on supported platforms.
- Around line 36-47: Update the environment setup block around the `key` and
`value` buffers to free both `toNativeUtf8()` allocations with
`malloc.free(...)` instead of `calloc.free(...)`, including the `finally`
cleanup for `key`.
In `@test/db_p0_fixes_test.dart`:
- Around line 224-232: Make the database tests order-independent by moving
full-table cleanup for events, day_result, decoded_onehz, and decoded_rr into
shared setUp logic, or otherwise isolating each test’s data with unique
counters/day IDs. Update the tests identified by fix 11, fix 10, fix 8, and fix
1 while preserving their existing assertions and setup behavior.
In `@test/derive_day_window_test.dart`:
- Around line 108-116: Remove the tautological loop comparing
tzOffsetSecondsAt(t) with the identical DateTime expression; retain the jan/jul
divergence assertion below as the meaningful coverage for seasonal offset
behavior.
In `@test/derive_result_protection_test.dart`:
- Around line 286-287: Rename the test case describing skip-marker replacement
to remove the contradictory “does not overwrite” versus “is allowed to replace”
wording. Use a name that clearly states a skip marker may replace another skip
marker, matching the behavior asserted by the test body.
- Around line 162-167: Update the assertion around LocalDb.dayResult(freshDay)
so the test first requires row to be non-null, then asserts row['finalized']
equals 0 without a conditional guard. Preserve the existing reason text and
ensure a missing result causes the test to fail.
In `@test/telemetry_consent_default_test.dart`:
- Around line 30-36: Update plistValueAfter to parse both self-closing values
and paired element content, returning the actual value rather than the tag name.
Relax the manifest assertion regex so android:name and android:value may appear
in either order with intervening attributes, while preserving validation of the
expected configuration.
🪄 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: 1b5d7715-986d-42cf-a958-4a605c206bcb
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (77)
android/app/src/main/AndroidManifest.xmlandroid/app/src/main/kotlin/wtf/openstrap/openstrap_edge/OpenStrapWidgetProvider.ktios/OpenStrapWatch Watch App/WatchMetrics.swiftios/OpenStrapWidget/OpenStrapWidget.swiftios/Runner/Info.plistlib/app.dartlib/ble/ble_engine.dartlib/ble/ble_state.dartlib/cloud/cloud_import.dartlib/coach/coach_db.dartlib/coach/coach_engine.dartlib/coach/coach_prompt.dartlib/compute/crossday_pipeline.dartlib/compute/derivation_engine.dartlib/compute/derive_prepare.dartlib/compute/onehz_pipeline.dartlib/compute/substrate.dartlib/data/day_label.dartlib/data/db.dartlib/data/local_repository_impl.dartlib/gps/route_math.dartlib/gps/route_tracker.dartlib/import/noop_import.dartlib/import/whoop_import.dartlib/main.dartlib/notify/notification_center.dartlib/notify/notification_event.dartlib/notify/notification_ids.dartlib/notify/notification_service.dartlib/state/app_state.dartlib/sync/headless_boot.dartlib/sync/sync_policy.dartlib/telemetry/telemetry_service.dartlib/ui/ai/ai_breakdown_screen.dartlib/ui/ai/ai_settings_screen.dartlib/ui/coach/ai_coach_screen.dartlib/ui/coach/coach_chart.dartlib/ui/design/recap_card.dartlib/ui/kit/charts.dartlib/ui/kit/kit.dartlib/ui/screens/detail_cards.dartlib/ui/screens/metric_screen.dartlib/ui/screens/screens.dartlib/ui/screens/trend_screen.dartlib/ui/widgets/async_guards.dartlib/widget/widget_service.dartpubspec.yamltest/absent_not_zero_test.darttest/ai_screen_async_guards_test.darttest/app_state_regressions_test.darttest/ble_engine_test.darttest/ble_safe_trim_test.darttest/ble_transport_guards_test.darttest/coach_provider_response_test.darttest/coach_sql_guard_adversarial_test.darttest/coach_sql_structural_test.darttest/crossday_pipeline_test.darttest/day_window_dst_test.darttest/db_migration_ladder_test.darttest/db_p0_fixes_test.darttest/derive_day_window_test.darttest/derive_isolate_lifecycle_test.darttest/derive_result_protection_test.darttest/headless_gate_test.darttest/import_data_safety_test.darttest/local_repository_p0_test.darttest/metric_trend_redesign_test.darttest/notification_center_test.darttest/notification_day_guard_test.darttest/notification_ids_test.darttest/notification_permission_test.darttest/notification_schedule_dst_test.darttest/readiness_baseline_pollution_test.darttest/route_math_test.darttest/route_tracker_test.darttest/telemetry_consent_default_test.darttest/widget_service_sentinels_test.dart
💤 Files with no reviewable changes (1)
- test/ble_engine_test.dart
| static Future<Set<int>> _allowedRootPages(Database db) async { | ||
| final cached = _allowedRoots; | ||
| if (cached != null) return cached; | ||
| final roots = <int>{}; | ||
| for (final v in allowedViews) { | ||
| try { | ||
| roots.addAll(await _btreeRoots(db, 'SELECT * FROM $v')); | ||
| } catch (_) {/* a view that can't compile contributes nothing */} | ||
| } | ||
| // Indexes on an allowed base table are allowed too (the planner may pick | ||
| // one for a WHERE the plain scan above never used). | ||
| final schema = await db.rawQuery( | ||
| 'SELECT type, name, tbl_name, rootpage FROM sqlite_master'); | ||
| final tableOfRoot = <int, String>{}; | ||
| for (final r in schema) { | ||
| final rp = (r['rootpage'] as num?)?.toInt(); | ||
| final tbl = r['tbl_name']?.toString(); | ||
| if (rp != null && rp > 0 && tbl != null) tableOfRoot[rp] = tbl; | ||
| } | ||
| final allowedTables = <String>{ | ||
| for (final rp in roots) | ||
| if (tableOfRoot[rp] != null) tableOfRoot[rp]!, | ||
| }; | ||
| for (final e in tableOfRoot.entries) { | ||
| if (allowedTables.contains(e.value)) roots.add(e.key); | ||
| } | ||
| _allowedRoots = roots; | ||
| return roots; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Root-page cache is not invalidated when the schema changes.
_allowedRoots is computed once per process and only cleared in close(). A VACUUM, migration, or index create/drop on the RW handle (LocalDb) renumbers root pages, after which the cached set no longer describes the allowed views — stale entries could allow a btree that now belongs to a different table, and legitimate view queries would be rejected. Consider keying the cache on PRAGMA schema_version (read through rawQuery) or clearing it whenever LocalDb runs a migration.
🤖 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/coach/coach_db.dart` around lines 390 - 418, The _allowedRootPages cache
must be invalidated when the SQLite schema changes. Update _allowedRootPages to
read PRAGMA schema_version through rawQuery and associate _allowedRoots with
that version, recomputing roots whenever the version differs; preserve the
existing fast path when the cached version still matches.
| /// Hard ceiling on one serialized provider request body. | ||
| static const int kMaxRequestBytes = 400 * 1024; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
kMaxRequestBytes is compared against a UTF-16 code-unit count, not bytes.
payload.length counts Dart string code units; the request is sent as UTF-8. Non-ASCII content (journal notes, emoji, non-Latin text) can be up to 3× larger on the wire, so the "hard ceiling on bytes leaving the device" can be exceeded. The user-facing message also reports payload.length ~/ 1024 as "KB".
🔒️ Measure the encoded payload
- final payload = jsonEncode(body);
- if (payload.length > kMaxRequestBytes) {
+ final payload = jsonEncode(body);
+ final payloadBytes = utf8.encode(payload).length;
+ if (payloadBytes > kMaxRequestBytes) {
throw CoachException(
- 'That request grew to ${payload.length ~/ 1024} KB, over the '
+ 'That request grew to ${payloadBytes ~/ 1024} KB, over the '
'${kMaxRequestBytes ~/ 1024} KB safety limit for data leaving this '
'device. Start a new chat or ask a narrower question (aggregate with '
'AVG/MIN/MAX/COUNT instead of selecting every row).');
}Then send body: utf8.encode(payload) (or keep payload; http encodes as UTF-8 by default) — the check is the part that must match the wire size.
Also applies to: 590-597
🤖 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/coach/coach_engine.dart` around lines 212 - 213, Update the request-size
enforcement around kMaxRequestBytes to measure the UTF-8 encoded payload length
rather than payload.length, and use that same byte count for the user-facing KB
value. Preserve the existing limit and rejection behavior, and send the encoded
bytes via the request body or otherwise ensure the transmitted body uses the
measured UTF-8 representation.
| /// Parse a 'YYYY-MM-DD' label into its (year, month, day) parts, or null if it | ||
| /// isn't one. Deliberately strict — a malformed label must not silently become | ||
| /// epoch 0 and delete/export the wrong window. | ||
| List<int>? _partsOf(String dayId) { | ||
| final p = dayId.split('-'); | ||
| if (p.length != 3) return null; | ||
| final y = int.tryParse(p[0]); | ||
| final m = int.tryParse(p[1]); | ||
| final d = int.tryParse(p[2]); | ||
| if (y == null || m == null || d == null) return null; | ||
| return [y, m, d]; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
_partsOf accepts out-of-range components, so "strict" isn't strict.
Shape and numeric parsing are checked, but ranges are not: '2026-13-45' or '2026-02-31' pass and DateTime silently normalizes them to a different calendar day. Since localDayStartSec/localDayEndSec bound day-window deletes and exports (see lib/data/db.dart around lines 2916-2917), a normalized label targets the wrong window — the exact failure mode the doc comment says this guards against.
🛡️ Reject out-of-range components
final y = int.tryParse(p[0]);
final m = int.tryParse(p[1]);
final d = int.tryParse(p[2]);
if (y == null || m == null || d == null) return null;
+ if (m < 1 || m > 12 || d < 1 || d > 31) return null;
+ // Reject a day that does not exist in that month (DateTime would silently
+ // roll it into the next one).
+ if (DateTime(y, m, d).day != d) return null;
return [y, m, d];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Parse a 'YYYY-MM-DD' label into its (year, month, day) parts, or null if it | |
| /// isn't one. Deliberately strict — a malformed label must not silently become | |
| /// epoch 0 and delete/export the wrong window. | |
| List<int>? _partsOf(String dayId) { | |
| final p = dayId.split('-'); | |
| if (p.length != 3) return null; | |
| final y = int.tryParse(p[0]); | |
| final m = int.tryParse(p[1]); | |
| final d = int.tryParse(p[2]); | |
| if (y == null || m == null || d == null) return null; | |
| return [y, m, d]; | |
| } | |
| /// Parse a 'YYYY-MM-DD' label into its (year, month, day) parts, or null if it | |
| /// isn't one. Deliberately strict — a malformed label must not silently become | |
| /// epoch 0 and delete/export the wrong window. | |
| List<int>? _partsOf(String dayId) { | |
| final p = dayId.split('-'); | |
| if (p.length != 3) return null; | |
| final y = int.tryParse(p[0]); | |
| final m = int.tryParse(p[1]); | |
| final d = int.tryParse(p[2]); | |
| if (y == null || m == null || d == null) return null; | |
| if (m < 1 || m > 12 || d < 1 || d > 31) return null; | |
| // Reject a day that does not exist in that month (DateTime would silently | |
| // roll it into the next one). | |
| if (DateTime(y, m, d).day != d) return null; | |
| return [y, m, d]; | |
| } |
🤖 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/data/day_label.dart` around lines 26 - 37, Update _partsOf to validate
that the parsed year, month, and day form the exact requested calendar date,
rejecting out-of-range values such as invalid months or days instead of allowing
DateTime normalization. Preserve the existing null result for malformed labels
and return the parsed components only when they represent a valid date.
| /// The live schema version — the ONE place it is declared. Every | ||
| /// `openDatabase` this class performs (the app DB and the day-export DB) must | ||
| /// pass it: sqflite throws `ArgumentError('onCreate must be null if no | ||
| /// version is specified')` BEFORE opening anything when `onCreate` is given | ||
| /// without `version` (sqflite_common database_mixin.dart). | ||
| static const int schemaVersion = 26; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
schemaVersion isn't yet the single source — _open() still passes the literal 26.
The doc promises "Every openDatabase this class performs … must pass it", but the app-DB open at Line 397 still hardcodes version: 26. The next bump has to be made in two places, and a missed one silently skips the whole onUpgrade ladder.
♻️ Use the constant at the app-DB open too (Line 397)
- version: 26,
+ version: schemaVersion,🤖 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/data/db.dart` around lines 88 - 93, Update the app-database open call in
_open() to pass the shared schemaVersion constant instead of the literal 26,
ensuring all database opens in the class use the single schema-version source.
Source: Coding guidelines
| static Future<void> _migrateLegacyTable( | ||
| Database db, { | ||
| required String table, | ||
| required bool Function(Set<String> columns) isCurrent, | ||
| required Future<void> Function(DatabaseExecutor ex) create, | ||
| required Future<void> Function( | ||
| DatabaseExecutor ex, | ||
| List<Map<String, Object?>> legacyRows, | ||
| int nowMs, | ||
| ) | ||
| copy, | ||
| bool copyOnlyIntoEmpty = false, | ||
| }) async { | ||
| final legacy = '${table}_legacy'; | ||
| await db.transaction((txn) async { | ||
| Future<bool> exists(String t) async => | ||
| (await txn.rawQuery( | ||
| "SELECT name FROM sqlite_master WHERE type = 'table' AND name = ?", | ||
| [t], | ||
| )).isNotEmpty; | ||
| Future<Set<String>> columnsOf(String t) async { | ||
| final info = await txn.rawQuery('PRAGMA table_info($t)'); | ||
| return { | ||
| for (final c in info) | ||
| if (c['name'] is String) c['name'] as String, | ||
| }; | ||
| } | ||
|
|
||
| await db.execute('ALTER TABLE sync_ledger RENAME TO sync_ledger_legacy'); | ||
| await _createSyncState(db); | ||
| final legacyRows = await db.query('sync_ledger_legacy'); | ||
| final now = DateTime.now().millisecondsSinceEpoch; | ||
| for (final row in legacyRows) { | ||
| final meta = <String, dynamic>{ | ||
| 'last_batch_token': row['last_batch_token'], | ||
| 'last_batch_id': row['last_batch_id'], | ||
| 'last_batch_records': row['last_batch_records'], | ||
| 'last_history_complete_at': row['last_history_complete_at'], | ||
| 'last_trim_cutoff_ms': row['last_trim_cutoff_ms'], | ||
| 'last_trimmed_at': row['last_trimmed_at'], | ||
| if (row['note'] != null) 'legacy_note': row['note'], | ||
| }; | ||
| await db.insert('sync_ledger', { | ||
| 'chunk_id': (row['id'] as String?) ?? 'capture', | ||
| 'kind': 'historical', | ||
| 'status': row['last_history_complete_at'] != null | ||
| ? 'complete' | ||
| : row['last_batch_acked_at'] != null | ||
| ? 'acknowledged' | ||
| : 'legacy', | ||
| 'created_at': (row['updated_at'] as num?)?.toInt() ?? now, | ||
| 'updated_at': (row['updated_at'] as num?)?.toInt() ?? now, | ||
| 'acked_at': (row['last_batch_acked_at'] as num?)?.toInt(), | ||
| 'last_error': null, | ||
| 'meta_json': jsonEncode(meta), | ||
| }, conflictAlgorithm: ConflictAlgorithm.replace); | ||
| } | ||
| await db.execute('DROP TABLE sync_ledger_legacy'); | ||
| if (!await exists(legacy)) { | ||
| // Normal path. | ||
| if (!await exists(table)) { | ||
| await create(txn); | ||
| return; | ||
| } | ||
| if (isCurrent(await columnsOf(table))) return; | ||
| await txn.execute('ALTER TABLE $table RENAME TO $legacy'); | ||
| } | ||
| // From here on `<table>_legacy` holds the rows of record. `<table>` is | ||
| // either absent (crash between RENAME and CREATE) or the new shape | ||
| // (possibly half-copied, or fully copied by an older build that then | ||
| // died before the DROP) — create it if needed and re-copy; the copy is | ||
| // idempotent, so a repeat is a no-op rather than a duplication. | ||
| if (!await exists(table)) await create(txn); | ||
| final skipCopy = | ||
| copyOnlyIntoEmpty && | ||
| (Sqflite.firstIntValue( | ||
| await txn.rawQuery('SELECT COUNT(*) FROM $table'), | ||
| ) ?? | ||
| 0) > | ||
| 0; | ||
| if (!skipCopy) { | ||
| await copy( | ||
| txn, | ||
| await txn.query(legacy), | ||
| DateTime.now().millisecondsSinceEpoch, | ||
| ); | ||
| } | ||
| await txn.execute('DROP TABLE $legacy'); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
sqflite nested db.transaction inside onUpgrade onOpen deadlock behavior
💡 Result:
In sqflite, callbacks such as onCreate, onUpgrade, and onDowngrade are already executed within an internal transaction [1][2][3]. Attempting to open a nested transaction (by calling database.transaction(...) inside these callbacks) is strongly discouraged, as sqflite and underlying SQLite implementations generally do not support true nested transactions, which frequently leads to deadlocks or "database locked" errors [2][3][4]. Key points regarding this behavior: 1. Automatic Transaction Wrapping: You do not need to wrap your migration logic in a transaction [2][3]. All SQL statements executed directly on the database object within onUpgrade or onCreate are automatically part of the same transaction [5][1]. 2. Deadlock Mechanism: A deadlock occurs when a transaction is already active and you attempt to perform operations using the main database object instead of the provided transaction object [6][2][7]. Because these callbacks are already inside a transaction, any attempt to initiate a new transaction or use the database object for conflicting operations will block, as sqflite synchronizes access to the database to ensure exclusive write operations [2][3][8]. 3. Proper Usage: Always use the database object directly for migrations within onUpgrade/onCreate [5][1]. If you need to perform multiple operations, you can use the database object (which is already transactional) or a batch for better performance [5]. 4. onOpen: Unlike onUpgrade or onCreate, the onOpen callback is called after the database version is set and is generally intended for non-transactional database setup tasks, such as enabling foreign key support or printing version information [1]. If you must perform transactional operations during onOpen, you must explicitly start a transaction and ensure all operations within that block use the provided Transaction object, not the main database object [6][2][7]. Using the main database object inside a transaction will cause a deadlock [2][7][9].
Citations:
- 1: https://github.com/tekartik/sqflite/blob/master/sqflite/doc/opening_db.md
- 2: https://pub.dev/documentation/sqflite/latest/
- 3: https://pub.dev/packages/sqflite
- 4: https://stackoverflow.com/questions/59021882/android-sqlite-database-locked-exception-on-first-getwritabledatabase-execu
- 5: Direct transaction access in onUpgrade tekartik/sqflite#944
- 6: https://github.com/tekartik/sqflite/blob/master/sqflite/doc/troubleshooting.md
- 7: https://github.com/tekartik/sqflite/blob/master/sqflite/doc/sql.md
- 8: https://pub.dev/documentation/sqflite/
- 9: https://pub.dev/documentation/sqflite/latest/sqflite/Database-class.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the migration helper and its callers, then inspect the surrounding lines.
file="lib/data/db.dart"
echo "== Outline =="
ast-grep outline "$file" --view expanded || true
echo
echo "== Relevant line ranges =="
sed -n '1360,1585p' "$file" | cat -n
echo
echo "== Call sites =="
rg -n "_migrateLegacyTable|_ensureSyncStateSchema|_repairOpenSchema|onUpgrade|onOpen" "$file"Repository: OpenStrap/edge
Length of output: 12482
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="lib/data/db.dart"
sed -n '1478,1575p' "$file" | cat -nRepository: OpenStrap/edge
Length of output: 4734
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="lib/data/db.dart"
# Show the caller code for _ensureSyncStateSchema / _repairOpenSchema and the db.open callbacks.
sed -n '1478,1605p' "$file" | cat -nRepository: OpenStrap/edge
Length of output: 6098
🌐 Web query:
sqflite transaction inside onUpgrade same database object deadlock docs nested transaction
💡 Result:
In sqflite, you should not manually start a transaction within onUpgrade, onCreate, or onDowngrade [1][2]. These callbacks are already internally wrapped in a transaction managed by the library [3][1][2]. If you attempt to perform database operations inside these callbacks, you must use the Database object (the db parameter passed to the callback) directly, not a transaction object [4][3]. Because these lifecycle methods already operate within a transaction, any attempt to call db.transaction(...) inside them or to use the main database object instead of the provided transaction-specific interface within an existing transaction context is a primary cause of deadlocks [5][6][1]. Key Technical Points: - Avoid Nested Transactions: SQLite itself does not support true nested transactions via BEGIN...COMMIT [7]. While you can use SAVEPOINT and RELEASE, sqflite simplifies this by managing the lifecycle transaction for you [3][1]. - Use the Correct Object: The fundamental rule to avoid deadlocks in sqflite is to use the transaction object (typically named txn) for all operations inside a transaction block [5][6][8]. However, since onUpgrade/onCreate/onDowngrade are already executing inside a library-managed transaction, simply use the provided Database instance [4][3][1]. - Deadlock Prevention: A deadlock occurs when you use the main database object to perform an operation while a transaction is already active [5][1]. Since the library has already locked the database to perform the migration, attempting to start another transaction—or using the main db object to interact with the database from within that context—will lead to the classic "database has been locked" timeout error [5][6][1]. In summary, treat the db parameter in onUpgrade as your sole interface for all migrations and refrain from wrapping your logic in additional db.transaction calls [4][3][1].
Citations:
- 1: https://github.com/tekartik/sqflite/blob/master/sqflite/README.md
- 2: https://pub.dev/documentation/sqflite/latest/
- 3: https://github.com/tekartik/sqflite/blob/master/sqflite/doc/opening_db.md
- 4: Direct transaction access in onUpgrade tekartik/sqflite#944
- 5: https://github.com/tekartik/sqflite/blob/master/sqflite/doc/troubleshooting.md
- 6: Warning database has been locked for 0:00:10.000000. Make sure you always use the transaction object for database operations during a transaction tekartik/sqflite#921
- 7: https://sqlite.org/lang_transaction.html
- 8: https://github.com/tekartik/sqflite/blob/master/sqflite/doc/sql.md
Avoid nested db.transaction in _migrateLegacyTable (lib/data/db.dart:1417-1476)
onUpgrade already runs inside sqflite’s migration transaction, so wrapping it again here can deadlock or stall database open. Use the ambient executor on the upgrade path and keep the explicit transaction only for the onOpen repair path.
🤖 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/data/db.dart` around lines 1417 - 1476, Update _migrateLegacyTable to
accept and operate on the ambient DatabaseExecutor when invoked from onUpgrade,
avoiding a nested db.transaction; retain an explicit transaction only for the
onOpen repair path. Preserve the existing migration, copy, skipCopy, and
legacy-table cleanup behavior while routing both paths through the shared logic.
| // Finalizing locks a day out of DerivationEngine permanently. Only safe | ||
| // when there is no raw substrate left to re-derive from; a day that still | ||
| // has 1 Hz raw stays open so the real signal supersedes WHOOP's numbers. | ||
| finalized: !rawDays.contains(date), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
rawDays is "raw on device now", not "raw that will ever exist" — this can still permanently lock a day.
rawDays is snapshotted from LocalDb.decodedRecTsMaxByDay() before the loop, i.e. only days already drained from the band. The import is reachable from onboarding, which typically runs before the first sync: at that point rawDays is empty (or nearly so), every exported day is written finalized: true, and when the band's flash is subsequently drained for those same dates DerivationEngine can never derive them — finalizedDayIds(kAlgoVersion) removes them from todoDays, and per putDayResult's own contract in lib/data/db.dart a finalized row is never revisited on a version bump either.
The comment's premise ("no raw substrate left to re-derive from") only holds for dates older than the band's retention window; for anything newer, absence of raw today is not evidence.
🛡️ Only finalize dates that can no longer receive raw
- // Finalizing locks a day out of DerivationEngine permanently. Only safe
- // when there is no raw substrate left to re-derive from; a day that still
- // has 1 Hz raw stays open so the real signal supersedes WHOOP's numbers.
- finalized: !rawDays.contains(date),
+ // Finalizing locks a day out of DerivationEngine permanently, so never
+ // do it for a date the band could still deliver raw for. `rawDays` only
+ // knows what has ALREADY been drained — on the onboarding path that is
+ // empty, so a bare `!rawDays.contains(date)` would lock every imported
+ // day before the first sync. Leave imports open; the `producedNothing`
+ // guard in DerivationEngine already stops an empty re-derive from
+ // blanking them.
+ finalized: false,If keeping finalization for genuinely historical dates is preferred, gate it on the data edge instead — e.g. only when date is older than lastDecodedRecTs() - rawRetentionDays, which is the same rule _pruneOldDecoded uses.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Finalizing locks a day out of DerivationEngine permanently. Only safe | |
| // when there is no raw substrate left to re-derive from; a day that still | |
| // has 1 Hz raw stays open so the real signal supersedes WHOOP's numbers. | |
| finalized: !rawDays.contains(date), | |
| // Finalizing locks a day out of DerivationEngine permanently, so never | |
| // do it for a date the band could still deliver raw for. `rawDays` only | |
| // knows what has ALREADY been drained — on the onboarding path that is | |
| // empty, so a bare `!rawDays.contains(date)` would lock every imported | |
| // day before the first sync. Leave imports open; the `producedNothing` | |
| // guard in DerivationEngine already stops an empty re-derive from | |
| // blanking them. | |
| finalized: false, |
🤖 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/import/whoop_import.dart` around lines 255 - 258, Update the finalized
assignment in the WHOOP import loop to finalize a date only when it is older
than the device’s raw-data retention boundary, rather than when it is merely
absent from the snapshotted rawDays set. Reuse the existing lastDecodedRecTs()
and raw-retention calculation used by _pruneOldDecoded, ensuring dates still
able to receive future raw data remain unfinalized.
| @override | ||
| void dispose() { | ||
| // EVERY timer this object owns, not just three of them. _spotTimer, | ||
| // _breathingRecomputeTimer and _workoutTimer used to survive dispose, and | ||
| // each of their callbacks ends in notifyListeners() on a disposed | ||
| // ChangeNotifier (which throws in release). | ||
| _tapSub?.cancel(); | ||
| _stopBackfillTimer(); | ||
| _alarmGraceTimer?.cancel(); | ||
| _alarmGraceTimer = null; | ||
| _spotTimer?.cancel(); | ||
| _spotTimer = null; | ||
| _breathingRecomputeTimer?.cancel(); | ||
| _breathingRecomputeTimer = null; | ||
| _workoutTimer?.cancel(); | ||
| _workoutTimer = null; | ||
| BandOwnership.markForegroundIntent(false); | ||
| _releaseForegroundLease(); | ||
| _deriveScheduler.dispose(); | ||
| _waterBuzzer.dispose(); | ||
| // Owned notifiers/observers. notificationRelay in particular holds a | ||
| // WidgetsBindingObserver, a 120 s Timer.periodic and a StreamSubscription — | ||
| // its observer accumulated on the binding across every hot restart. | ||
| notificationRelay.dispose(); | ||
| gestureSettings.dispose(); | ||
| navRequest.dispose(); | ||
| screenRequest.dispose(); | ||
| insightsRevision.dispose(); | ||
| super.dispose(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
_routeTracker isn't torn down here.
The stated goal of this hunk is "EVERY timer this object owns", but a live RouteTracker (GPS stream subscription + buffered points flushed via LocalDb.appendRoutePoints) survives dispose() when the app is disposed mid-route-workout — same leak shape as the timers you just fixed, and its sink writes against a dead AppState.
🛡️ Proposed fix
_workoutTimer?.cancel();
_workoutTimer = null;
+ final rt = _routeTracker;
+ _routeTracker = null;
+ if (rt != null) unawaited(rt.stop().catchError((_) {}));
BandOwnership.markForegroundIntent(false);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @override | |
| void dispose() { | |
| // EVERY timer this object owns, not just three of them. _spotTimer, | |
| // _breathingRecomputeTimer and _workoutTimer used to survive dispose, and | |
| // each of their callbacks ends in notifyListeners() on a disposed | |
| // ChangeNotifier (which throws in release). | |
| _tapSub?.cancel(); | |
| _stopBackfillTimer(); | |
| _alarmGraceTimer?.cancel(); | |
| _alarmGraceTimer = null; | |
| _spotTimer?.cancel(); | |
| _spotTimer = null; | |
| _breathingRecomputeTimer?.cancel(); | |
| _breathingRecomputeTimer = null; | |
| _workoutTimer?.cancel(); | |
| _workoutTimer = null; | |
| BandOwnership.markForegroundIntent(false); | |
| _releaseForegroundLease(); | |
| _deriveScheduler.dispose(); | |
| _waterBuzzer.dispose(); | |
| // Owned notifiers/observers. notificationRelay in particular holds a | |
| // WidgetsBindingObserver, a 120 s Timer.periodic and a StreamSubscription — | |
| // its observer accumulated on the binding across every hot restart. | |
| notificationRelay.dispose(); | |
| gestureSettings.dispose(); | |
| navRequest.dispose(); | |
| screenRequest.dispose(); | |
| insightsRevision.dispose(); | |
| super.dispose(); | |
| } | |
| `@override` | |
| void dispose() { | |
| // EVERY timer this object owns, not just three of them. _spotTimer, | |
| // _breathingRecomputeTimer and _workoutTimer used to survive dispose, and | |
| // each of their callbacks ends in notifyListeners() on a disposed | |
| // ChangeNotifier (which throws in release). | |
| _tapSub?.cancel(); | |
| _stopBackfillTimer(); | |
| _alarmGraceTimer?.cancel(); | |
| _alarmGraceTimer = null; | |
| _spotTimer?.cancel(); | |
| _spotTimer = null; | |
| _breathingRecomputeTimer?.cancel(); | |
| _breathingRecomputeTimer = null; | |
| _workoutTimer?.cancel(); | |
| _workoutTimer = null; | |
| final rt = _routeTracker; | |
| _routeTracker = null; | |
| if (rt != null) unawaited(rt.stop().catchError((_) {})); | |
| BandOwnership.markForegroundIntent(false); | |
| _releaseForegroundLease(); | |
| _deriveScheduler.dispose(); | |
| _waterBuzzer.dispose(); | |
| // Owned notifiers/observers. notificationRelay in particular holds a | |
| // WidgetsBindingObserver, a 120 s Timer.periodic and a StreamSubscription — | |
| // its observer accumulated on the binding across every hot restart. | |
| notificationRelay.dispose(); | |
| gestureSettings.dispose(); | |
| navRequest.dispose(); | |
| screenRequest.dispose(); | |
| insightsRevision.dispose(); | |
| super.dispose(); | |
| } |
🤖 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/state/app_state.dart` around lines 750 - 779, Update AppState.dispose()
to tear down the owned _routeTracker before super.dispose(), canceling or
disposing it through the RouteTracker lifecycle API and clearing the field if
applicable. Ensure its GPS subscription and buffered-point flush cannot continue
writing after AppState disposal, while preserving the existing cleanup order for
the other owned resources.
| Future<bool?> runBootSyncThroughGate( | ||
| BandLease lease, { | ||
| Future<bool> Function(BandLease lease)? runner, | ||
| }) async { | ||
| final run = runner ?? ((l) => runHeadlessSync(lease: l)); | ||
| final result = await HeadlessSyncGate.tryRun<bool>( | ||
| kBootWakeGateOwner, | ||
| () => run(lease), | ||
| ); | ||
| if (result == null) { | ||
| // Skipped — release the lease we acquired up front, or the band stays | ||
| // owned by a headless run that never happened. | ||
| BandOwnership.release(lease); | ||
| } | ||
| return result; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Boot-wake failure path leaves the band lease held and the error unhandled. Both sites stem from one root cause: runBootSyncThroughGate only releases the BandLease on the gate-busy (null) path, so a throwing run strands ownership and escapes the fire-and-forget chain.
lib/sync/headless_boot.dart#L47-L62: wrap theHeadlessSyncGate.tryRuncall in try/catch, release the lease, and rethrow.lib/sync/headless_boot.dart#L113-L124: add anonError(orcatchError) to theunawaited(...)chain so a failure logs instead of becoming an unhandled async error in the headless isolate.
📍 Affects 1 file
lib/sync/headless_boot.dart#L47-L62(this comment)lib/sync/headless_boot.dart#L113-L124
🤖 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/sync/headless_boot.dart` around lines 47 - 62, Update
runBootSyncThroughGate to catch failures from HeadlessSyncGate.tryRun, release
the acquired BandLease, and rethrow the original error. Also update the
unawaited(...) chain at lib/sync/headless_boot.dart lines 113-124 to handle
errors with onError or catchError and log them, preventing unhandled async
failures; both locations require changes.
| // Fire-and-forget (see the doc above) but SERIALISED through the shared | ||
| // headless gate, so `HeadlessSyncGate.busy` is true for the whole boot drain. | ||
| unawaited( | ||
| runBootSyncThroughGate(lease).then((ran) { | ||
| debugPrint( | ||
| ran == null | ||
| ? '[headless-boot] boot wake skipped — another headless sync ' | ||
| 'holds the gate; lease released.' | ||
| : '[headless-boot] headless sync complete', | ||
| ); | ||
| }), | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Add an onError to the fire-and-forget chain.
.then(...) alone lets any failure from runBootSyncThroughGate escape as an unhandled async error in a headless isolate (root cause covered in the comment on Lines 47-62).
🤖 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/sync/headless_boot.dart` around lines 113 - 124, Add an onError handler
to the fire-and-forget Future chain around runBootSyncThroughGate(lease) in the
headless boot flow, ensuring failures are handled rather than escaping as
unhandled async errors. Preserve the existing then callback and its logging
behavior, and report the caught error through the established debugging or
logging mechanism.
| set enabled(bool value) { | ||
| _enabled = value; | ||
| _consentResolved = true; | ||
| _applyFirebaseCollection(value); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 \
'setCrashlyticsCollectionEnabled|applyConsent|enforceCollectionOffUntilConsent|deleteUnsentReports' \
lib android ios test
rg -n \
'firebase_crashlytics|firebase_core' \
pubspec.yaml pubspec.lock 2>/dev/null || trueRepository: OpenStrap/edge
Length of output: 9437
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== lib/telemetry/telemetry_service.dart ==\n'
sed -n '1,180p' lib/telemetry/telemetry_service.dart
printf '\n== lib/main.dart ==\n'
sed -n '35,70p' lib/main.dart
printf '\n== lib/state/app_state.dart ==\n'
sed -n '425,490p' lib/state/app_state.dart
printf '\n== test/telemetry_consent_default_test.dart ==\n'
sed -n '1,180p' test/telemetry_consent_default_test.dartRepository: OpenStrap/edge
Length of output: 18681
🌐 Web query:
Firebase Crashlytics setCrashlyticsCollectionEnabled takes effect next app launch documentation
💡 Result:
Yes, the setCrashlyticsCollectionEnabled method in the Firebase Crashlytics SDK takes effect on the next launch of the app [1][2][3]. When you call this method, the new setting is persisted and will influence data collection behavior in subsequent sessions [4][3][5]. If you call this method to disable data collection (set to false) and cause a crash before the app is relaunched, the crash report may still be processed or sent [6]. To ensure the setting is respected, it is necessary to restart the application after the call [6][7]. Key details regarding this behavior: - Persistence: The value set via this method persists across all subsequent app launches for the user [4][3][5]. - Overriding: This runtime setting overrides other configurations, such as the manifest flag (Android) or the Info.plist flag (iOS) [1][3][5]. - Clearing the override: Passing a null value (on platforms where supported) will clear the runtime override, allowing the SDK to revert to your manifest or Info.plist configuration [1][2]. - Unsent reports: If you are disabling collection, you may also want to use deleteUnsentReports to remove any previously stored reports from the device if you do not intend to send them [1][7].
Citations:
- 1: https://firebase.google.com/docs/reference/android/com/google/firebase/crashlytics/FirebaseCrashlytics
- 2: https://firebase.google.com/docs/reference/kotlin/com/google/firebase/crashlytics/FirebaseCrashlytics
- 3: https://firebase.google.com/docs/reference/ios/firebasecrashlytics/api/reference/Classes/FIRCrashlytics
- 4: https://firebase.google.com/docs/crashlytics/android/customize-crash-reports
- 5: https://firebase.google.com/docs/reference/swift/firebasecrashlytics/api/reference/Classes/Crashlytics
- 6: Crashlytics still send crash report in debugging mode firebase/firebase-android-sdk#1481
- 7: https://stackoverflow.com/questions/60618036/firebase-crashlytics-collection-enabled-not-working-in-new-firebase-crashlytics
Revocation still needs a restart path. setCrashlyticsCollectionEnabled(false) only takes effect on the next app launch, so a user who opts out mid-session can still have crash data captured or uploaded for the rest of that process. Clear unsent reports on opt-out and block collection until the next cold start.
🤖 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/telemetry/telemetry_service.dart` around lines 46 - 50, Update the
enabled setter in the telemetry service to handle revocation immediately: when
value is false, clear any unsent Crashlytics reports, disable collection for the
remainder of the process, and preserve the restart-required behavior until the
next cold start. Keep the existing enable path and consent state updates intact.
CI failures (both real, neither caught locally): - `_denseDailyTrimp` turned "no observed load at all" into a synthetic all-zero series, so a user with no workouts got a confident CTL/ATL/TSB of 0 instead of an absent load model — the exact fabrication this sweep removes elsewhere. It surfaced only on CI because CI resolves the PINNED analytics (where ctlAtlTsb has no minimum-history gate) while local dev resolves the working copy through pubspec_overrides. Now abstains, which is correct against BOTH the pinned and the updated analytics, so the test no longer depends on which one is resolved. - The tsb/ctl-atl equality assertion used a 1e-6 tolerance. The three values round-trip through JSON independently, so the reconstructed difference can differ from the stored tsb by a ULP, and which way it lands is architecture-dependent: it passed on arm64 macOS and failed on x64 Linux by exactly 1e-6. Loosened to 1e-4, which still pins the relationship without asserting bit-identical floats across platforms. Review feedback (PR Agent, edge#158): - `_trimHistory` promised in its docstring that a `tool` message never outlives the assistant turn whose `tool_calls` it answers, but enforced it only as a side effect of the loop bounds. Both loops stop at `length > 1`, so a single turn larger than the whole byte budget walked the history down to one element: [assistant(tool_calls), tool] dropped the assistant and left the orphan. Providers 400 on that, and because the history is persisted the rejection repeats every turn — the session is bricked, not just one reply. Now enforced unconditionally.
|
Persistent review updated to latest commit 23aa39b |
…cord
real live-pedometer coverage
Diagnosed against a real user database, where the app reported 39,384 steps
for a day whose true value was ~2,000 (yesterday: 46).
The 1 Hz estimator keyed off ENMO, `mean(max(0, |a| - gRef))`, with gRef
auto-calibrated per day from the stillest samples — which are the long
sleep block, where the wrist sits in a different orientation than it does
awake. That day gRef came out at 0.9797 against ~1.032 on every other day
(hourly median |a| was 0.94-0.99 asleep vs 1.03-1.06 awake). ENMO subtracts
gRef from every sample, so a reference 0.05 g low adds 0.05 g to every
minute of the day — exactly the 0.05 g walking floor. Ordinary sitting
cleared the gate for hours: 368 "ambulatory" minutes on a partial day.
Sweeping gRef over the identical samples gave 42,155 steps at 0.97 and 0 at
1.02. The signal and the calibration error are the same size, so no
threshold could have fixed it; the analytics package now decides activity
from a per-axis high-passed dynamic amplitude, where any per-axis offset or
gain error cancels exactly.
This is the edge half.
PERSONAL FLOOR. The new feature needs to know what "moving" means for this
wearer, and analytics is pure so it cannot read history. Each day now
persists `dyn_p90` — its own high quantile of the dynamic amplitude — and
the next day's derive anchors on the MEDIAN across trailing days, with the
same strict self-exclusion every other baseline uses. Validated on the real
database: per-day values sit at 0.40-0.47 across ten days and the resulting
floor moves only between 0.4493 and 0.4575, absorbing a day whose own value
fell to 0.3148. A same-day floor was measured to fail in the opposite
direction (it collapses on a quiet day and passes everything), which is why
the anchor is multi-day and a median rather than a pooled quantile.
Below the minimum history there is NO floor and the estimator abstains, so
a new user sees the real 100 Hz count and no invented remainder. That
matters here: on this database Tier B contributed exactly 0 on every normal
day and 39,368 on the one drifted day — the stored total equalled the live
count exactly (46=46, 119+18+12=149, 105+201+103=409). It has never once
produced a useful number.
ACTIVE MINUTES. Persisted as a first-class series and surfaced as the
primary quantity. Gait is 1.4-2.5 Hz and 120 spm aliases to DC at 1 Hz, so
steps are not resolvable from this substrate; minutes are, and minutes of
moderate activity is the unit public activity guidance uses. Steps are now
reported as a range over the free-living cadence band.
LIVE COVERAGE. `live_coverage` windows were being written with
end_ts == start_ts — 22 of 24 rows in the real database, including one
claiming 1,657 real steps over 0 seconds. The window was derived purely
from the band record timestamp carried on live frames, which does not
advance across most sessions. Two consequences: the no-double-count
exclusion excluded ~1 minute instead of the streamed period, and there was
no way to align real 100 Hz counts to 1 Hz minutes — destroying the only
ground truth available to calibrate the estimator.
The window is now derived by a pure policy: anchored in the band record-time
base (what coverageWindowsOverlapping compares against), with the duration
taken from the 100 Hz sample count and the ingest wall hull, and raised to
the time the claimed steps could physically span. It can never be zero. The
persistence guard repairs a degenerate window rather than rejecting it, so
a real count is never lost. Historical rows are left alone — their true
durations were never recorded and inventing them would swap one wrong
extent for another.
kAlgoVersion 48 -> 49. The sibling analytics fixes are still NOT pinned;
pubspec.yaml must be repinned and this bumped again once that PR merges.
Added: personal ambulatory baseline + real live-pedometer coveragePushed Root cause is in analytics (the ENMO gravity-reference drift — details on that PR). Two things belong here: 1. Personal floor from trailing daysThe new calibration-invariant feature needs to know what "moving" means for this wearer, and analytics is pure so it cannot read history. Each day now persists Validated against the real database:
The floor barely moves even though 07-26's own value fell to 0.3148 — the median absorbs the anomalous day. That's the whole design goal: a same-day floor was measured to fail in the opposite direction (it collapses on a quiet day and passes everything). Below the minimum history there is no floor and the estimator abstains — a new user sees the real 100 Hz count and no invented remainder. That matters more than it sounds. On this database Tier B contributed exactly 0 on every normal day and 39,368 on the one drifted day — the stored total equalled the live count exactly (46=46, 119+18+12=149, 105+201+103=409). It has never once produced a useful number.
2.
|
|
Persistent review updated to latest commit b5c3352 |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/compute/derivation_engine.dart (1)
1152-1165: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound the worker-ready handshake too.
Line 1208 awaits
ready.futurewithout a timeout. If startup/handshake stalls before the worker sends itsSendPort, this pass can still hang indefinitely. Apply one_perDayTimeoutdeadline across bothready.futureandresult.future.🤖 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/compute/derivation_engine.dart` around lines 1152 - 1165, The worker startup path around derivationPrepareWorker must apply the same _perDayTimeout deadline to the ready.future handshake as to result.future. Update the await sequence so time spent waiting for the worker SendPort counts against one shared timeout budget, ensuring a stalled startup cannot hang indefinitely while preserving the existing result wait behavior.lib/state/app_state.dart (1)
3350-3359: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRe-check inside the reconciliation loop.
A stale-row
await LocalDb.putSession(...)can yield tostartWorkout(). A later valid row then overwrites the newly started workout and cancels its timer.Proposed fix
var resumed = false; for (final row in rows) { + if (activeWorkout != null) break; final startSec = (row['start_ts'] as num?)?.toInt();🤖 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/state/app_state.dart` around lines 3350 - 3359, Add the activeWorkout guard inside the reconciliation loop, immediately after each awaited LocalDb.putSession(...) operation and before applying the next database row. Return from the reconciliation method when activeWorkout is no longer null so startWorkout() remains authoritative and no stale row or timer replaces it.
🧹 Nitpick comments (4)
test/coach_history_trim_test.dart (1)
74-79: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
tool_calls: const []fixtures don't model a real assistant→tool pair.An empty
tool_callslist means the assistant issued no calls, so the followingtoolmessage withtool_call_id: 'call_1'is technically already an orphan. These fixtures only pass because the current sweep pairs by adjacency/role; if pairing is ever tightened to matchtool_call_id, they'd start failing for the wrong reason. Populate the id so the fixture is well-formed. Same pattern at Lines 94-95.♻️ Proposed fix
- {'role': 'assistant', 'content': null, 'tool_calls': const []}, + { + 'role': 'assistant', + 'content': null, + 'tool_calls': const [ + {'id': 'call_1', 'type': 'function'}, + ], + },🤖 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/coach_history_trim_test.dart` around lines 74 - 79, Update the assistant fixtures in the intact history case and the matching fixtures around the second occurrence to populate tool_calls with the call_1 identifier, so each subsequent tool message is associated with an explicitly issued call. Preserve the existing roles, content, and tool_call_id values.test/live_coverage_window_test.dart (1)
249-256: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThis assertion can pass vacuously.
'1970-01-01'is almost certainly not the day label the finalizer would use, so the query returns empty regardless of whether a window was written. Assert on the whole table instead.♻️ Proposed fix
- final rows = await db.query( - 'live_coverage', - where: 'day = ?', - whereArgs: ['1970-01-01'], - ); - expect(rows, isEmpty); + final rows = await db.query('live_coverage'); + expect(rows, isEmpty);Note this requires the table to be empty at that point; if ordering with other groups is a concern, capture the row count before
debugFinalizeLivePedometer()and assert it is unchanged.🤖 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/live_coverage_window_test.dart` around lines 249 - 256, Update the assertion after debugFinalizeLivePedometer() to validate the entire live_coverage table rather than querying the unlikely '1970-01-01' day label. Ensure the test verifies no rows were added, or capture the table row count before finalization and assert it remains unchanged if other test groups may already have data.test/step_personal_floor_test.dart (1)
86-92: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWeak assertion for the median-stability property.
lessThan(0.02)on the delta only proves the two floors are close; with 2 outliers appended to 7 samples the median genuinely shifts by one element. Assert the polluted floor stays within the normal range (e.g.lessThan(0.5)) as well, so a mean-based regression is caught even when the deltas happen to be small.🤖 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/step_personal_floor_test.dart` around lines 86 - 92, The median-stability test should also enforce that the polluted floor remains within the normal input range. In the test for personalDynFloorFromDailySummaries, retain the existing delta assertion and add an assertion that polluted is below the chosen upper bound (for example, lessThan(0.5)) to catch mean-based regressions.lib/state/app_state.dart (1)
1897-1902: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the shared local-day label helper.
Persist the coverage day with
dayLabelOf(DateTime.fromMillisecondsSinceEpoch(...))rather than manually formatting it.Proposed fix
- final d = DateTime.fromMillisecondsSinceEpoch(window.startTs * 1000); - final day = - '${d.year.toString().padLeft(4, '0')}-' - '${d.month.toString().padLeft(2, '0')}-' - '${d.day.toString().padLeft(2, '0')}'; + final day = dayLabelOf( + DateTime.fromMillisecondsSinceEpoch(window.startTs * 1000), + );As per coding guidelines, “Use todayLabel() or dayLabelOf() from data/day_label.dart for local day labels; do not derive labels from UTC strings.”
🤖 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/state/app_state.dart` around lines 1897 - 1902, Replace the manual day string construction in the coverage persistence flow with the shared dayLabelOf helper, passing the DateTime created from window.startTs. Remove the local year/month/day formatting while keeping the existing LocalDb.addLiveCoverage arguments unchanged.Source: Coding guidelines
🤖 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/compute/derivation_engine.dart`:
- Around line 2624-2663: The dailyStepEstimate abstention path must not expose
stale active minutes. In lib/compute/derivation_engine.dart lines 2624-2663,
clear or null scMap['active_min'] when v is null and only persist
estimator-derived active minutes when v is present; in lines 3550-3553, update
the wake synchronization to copy active_min alongside steps and calories,
including the absent/null case.
In `@lib/data/live_coverage_policy.dart`:
- Around line 121-130: The coverage calculation around duty-based selection must
not use aggregate sampled duration to anchor a sparse window at the session
start. Update the logic producing the sampled coverage interval so exact sampled
intervals are persisted and used when available; otherwise conservatively retain
hullS when duty is below kCoverageDutyFloor, preserving the existing hull
ceiling and zero-duration handling.
In `@test/coach_history_trim_test.dart`:
- Around line 66-68: Update the debugHistory assertions in the coach history
test to first require that engine.debugHistory is not empty, then
unconditionally verify its first entry’s role is not 'tool'. Remove the
conditional wrapper so an empty history causes the test to fail.
In `@test/step_personal_floor_test.dart`:
- Around line 22-31: The test depends on analytics APIs absent from the pinned
openstrap_analytics revision. Repin openstrap_analytics to the revision
containing personalDynFloorFromDailySummaries, personalDynFloorMinDays,
dailyDynSummary, personalDynFloorG, DailyStepEstimate.activeMinutes, and the
six-argument MotionMinute constructor; if unavailable immediately, gate or
disable this suite so flutter analyze and CI remain green.
---
Outside diff comments:
In `@lib/compute/derivation_engine.dart`:
- Around line 1152-1165: The worker startup path around derivationPrepareWorker
must apply the same _perDayTimeout deadline to the ready.future handshake as to
result.future. Update the await sequence so time spent waiting for the worker
SendPort counts against one shared timeout budget, ensuring a stalled startup
cannot hang indefinitely while preserving the existing result wait behavior.
In `@lib/state/app_state.dart`:
- Around line 3350-3359: Add the activeWorkout guard inside the reconciliation
loop, immediately after each awaited LocalDb.putSession(...) operation and
before applying the next database row. Return from the reconciliation method
when activeWorkout is no longer null so startWorkout() remains authoritative and
no stale row or timer replaces it.
---
Nitpick comments:
In `@lib/state/app_state.dart`:
- Around line 1897-1902: Replace the manual day string construction in the
coverage persistence flow with the shared dayLabelOf helper, passing the
DateTime created from window.startTs. Remove the local year/month/day formatting
while keeping the existing LocalDb.addLiveCoverage arguments unchanged.
In `@test/coach_history_trim_test.dart`:
- Around line 74-79: Update the assistant fixtures in the intact history case
and the matching fixtures around the second occurrence to populate tool_calls
with the call_1 identifier, so each subsequent tool message is associated with
an explicitly issued call. Preserve the existing roles, content, and
tool_call_id values.
In `@test/live_coverage_window_test.dart`:
- Around line 249-256: Update the assertion after debugFinalizeLivePedometer()
to validate the entire live_coverage table rather than querying the unlikely
'1970-01-01' day label. Ensure the test verifies no rows were added, or capture
the table row count before finalization and assert it remains unchanged if other
test groups may already have data.
In `@test/step_personal_floor_test.dart`:
- Around line 86-92: The median-stability test should also enforce that the
polluted floor remains within the normal input range. In the test for
personalDynFloorFromDailySummaries, retain the existing delta assertion and add
an assertion that polluted is below the chosen upper bound (for example,
lessThan(0.5)) to catch mean-based regressions.
🪄 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: c29bb031-9882-416d-9cc2-4159d14d9a26
📒 Files selected for processing (10)
lib/coach/coach_engine.dartlib/compute/crossday_pipeline.dartlib/compute/derivation_engine.dartlib/data/db.dartlib/data/live_coverage_policy.dartlib/state/app_state.darttest/coach_history_trim_test.darttest/crossday_pipeline_test.darttest/live_coverage_window_test.darttest/step_personal_floor_test.dart
🚧 Files skipped from review as they are similar to previous changes (4)
- lib/coach/coach_engine.dart
- test/crossday_pipeline_test.dart
- lib/compute/crossday_pipeline.dart
- lib/data/db.dart
| final v = est.present ? est.value : null; | ||
| final estSteps = v?.steps ?? 0; | ||
| final daySteps = liveStepsReal + estSteps; | ||
| scMap?['steps'] = daySteps.toDouble(); | ||
| // ACTIVE MINUTES is the primary, honest quantity here: 1 Hz cannot count | ||
| // steps (gait is 1.4-2.5 Hz and 120 spm aliases to DC at this rate), but | ||
| // it can resolve ambulatory MINUTES, which is also the unit public | ||
| // activity guidance is written in. The step figures are a RANGE over the | ||
| // free-living cadence band, and are absent entirely when the personal | ||
| // floor has not been established yet. | ||
| bundle['steps'] = <String, dynamic>{ | ||
| 'value': daySteps, | ||
| 'real_100hz': liveStepsReal, // AN-2554 over live windows (real count) | ||
| 'estimated_1hz': | ||
| estSteps, // walking-min × cadence for uncovered minutes | ||
| 'ambulatory_min': est.present ? est.value!.ambulatoryMinutes : 0, | ||
| 'cadence_used_spm': est.present ? est.value!.cadenceUsed : 0, | ||
| 'estimated_1hz': estSteps, // midpoint of the 1 Hz range | ||
| 'estimated_1hz_low': v?.stepsLow, | ||
| 'estimated_1hz_high': v?.stepsHigh, | ||
| 'active_min': v?.activeMinutes ?? 0, | ||
| 'cadence_low_spm': v?.cadenceLowSpm, | ||
| 'cadence_high_spm': v?.cadenceHighSpm, | ||
| 'dyn_floor_g': v?.dynFloorG, | ||
| 'estimate_present': v != null, | ||
| 'confidence': liveStepsReal > 0 | ||
| ? 0.7 | ||
| : (est.present ? est.confidence : 0.2), | ||
| 'tier': liveStepsReal > 0 && estSteps == 0 ? 'HIGH' : 'ESTIMATE', | ||
| 'inputs_used': const ['live_100hz_pedometer', 'enmo_1hz', 'hr_1hz'], | ||
| 'note': | ||
| 'real 100 Hz count for streamed time + 1 Hz walking estimate for ' | ||
| 'the rest (1 Hz cannot count steps directly)', | ||
| 'inputs_used': const [ | ||
| 'live_100hz_pedometer', | ||
| 'dyn_amp_1hz', | ||
| 'hr_1hz', | ||
| 'personal_dyn_floor', | ||
| ], | ||
| 'note': v == null | ||
| ? 'real 100 Hz count only — the 1 Hz activity estimate needs a ' | ||
| 'personal movement baseline from several days of wear ' | ||
| '(${est.note ?? 'need_baseline'})' | ||
| : 'real 100 Hz count for streamed time + ${v.activeMinutes} active ' | ||
| 'minutes estimated from 1 Hz for the rest (1 Hz cannot count ' | ||
| 'steps directly, so the step figure is a range)', | ||
| }; | ||
| if (v != null) scMap?['active_min'] = v.activeMinutes.toDouble(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve abstention for active_min across both persisted outputs.
When dailyStepEstimate abstains, active_min written earlier by _applyWakeDayFeatures remains in scMap; the early-read wake map also retains its older value. This exposes an activity estimate despite the missing personal baseline.
lib/compute/derivation_engine.dart#L2624-L2663: remove or nullscMap['active_min']whenv == null; only persist estimator-derived active minutes when present.lib/compute/derivation_engine.dart#L3550-L3553: synchronizeactive_minintowakealongside steps/calories, including the absent case.
🧰 Tools
🪛 GitHub Actions: test / 0_test.txt
[error] 2638-2638: flutter analyze: The getter 'stepsLow' isn't defined for type 'DailyStepEstimate'. (undefined_getter)
[error] 2639-2639: flutter analyze: The getter 'stepsHigh' isn't defined for type 'DailyStepEstimate'. (undefined_getter)
[error] 2640-2640: flutter analyze: The getter 'activeMinutes' isn't defined for type 'DailyStepEstimate'. (undefined_getter)
[error] 2641-2641: flutter analyze: The getter 'cadenceLowSpm' isn't defined for type 'DailyStepEstimate'. (undefined_getter)
[error] 2642-2642: flutter analyze: The getter 'cadenceHighSpm' isn't defined for type 'DailyStepEstimate'. (undefined_getter)
[error] 2643-2643: flutter analyze: The getter 'dynFloorG' isn't defined for type 'DailyStepEstimate'. (undefined_getter)
[error] 2659-2659: flutter analyze: The getter 'activeMinutes' isn't defined for type 'DailyStepEstimate'. (undefined_getter)
[error] 2663-2663: flutter analyze: The getter 'activeMinutes' isn't defined for type 'DailyStepEstimate'. (undefined_getter)
🪛 GitHub Actions: test / test
[error] 2638-2638: Getter 'stepsLow' isn't defined for type 'DailyStepEstimate'. (undefined_getter)
[error] 2639-2639: Getter 'stepsHigh' isn't defined for type 'DailyStepEstimate'. (undefined_getter)
[error] 2640-2640: Getter 'activeMinutes' isn't defined for type 'DailyStepEstimate'. (undefined_getter)
[error] 2641-2641: Getter 'cadenceLowSpm' isn't defined for type 'DailyStepEstimate'. (undefined_getter)
[error] 2642-2642: Getter 'cadenceHighSpm' isn't defined for type 'DailyStepEstimate'. (undefined_getter)
[error] 2643-2643: Getter 'dynFloorG' isn't defined for type 'DailyStepEstimate'. (undefined_getter)
[error] 2659-2659: Getter 'activeMinutes' isn't defined for type 'DailyStepEstimate'. (undefined_getter)
[error] 2663-2663: Getter 'activeMinutes' isn't defined for type 'DailyStepEstimate'. (undefined_getter)
📍 Affects 1 file
lib/compute/derivation_engine.dart#L2624-L2663(this comment)lib/compute/derivation_engine.dart#L3550-L3553
🤖 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/compute/derivation_engine.dart` around lines 2624 - 2663, The
dailyStepEstimate abstention path must not expose stale active minutes. In
lib/compute/derivation_engine.dart lines 2624-2663, clear or null
scMap['active_min'] when v is null and only persist estimator-derived active
minutes when v is present; in lines 3550-3553, update the wake synchronization
to copy active_min alongside steps and calories, including the absent/null case.
| if (hullS <= 0) { | ||
| coveredS = sampledS; // no hull observed — the sampled time is all we have | ||
| } else if (sampledS <= 0) { | ||
| coveredS = hullS; // steps without sample accounting — hull is the evidence | ||
| } else { | ||
| final duty = sampledS / hullS; | ||
| coveredS = duty >= kCoverageDutyFloor ? hullS : sampledS; | ||
| // The sampled time can exceed the hull (duplicate/backlogged frames). The | ||
| // hull is wall truth, so it also acts as the ceiling. | ||
| if (coveredS > hullS) coveredS = hullS; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not anchor sparse sampled duration at the session start.
When duty is below the floor, sampledS has no placement information. For example, samples concentrated at the end of a one-hour hull produce a short window starting at the first frame, so the actual live-step minute remains eligible for the 1 Hz estimate. Persist exact sampled intervals, or conservatively retain the hull when only aggregate timestamps are available.
🤖 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/data/live_coverage_policy.dart` around lines 121 - 130, The coverage
calculation around duty-based selection must not use aggregate sampled duration
to anchor a sparse window at the session start. Update the logic producing the
sampled coverage interval so exact sampled intervals are persisted and used when
available; otherwise conservatively retain hullS when duty is below
kCoverageDutyFloor, preserving the existing hull ceiling and zero-duration
handling.
Source: Coding guidelines
| if (engine.debugHistory.isNotEmpty) { | ||
| expect(engine.debugHistory.first['role'], isNot('tool')); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Conditional expect makes this test pass vacuously.
If a regression trims the history to empty, the body never runs and the test still passes. Assert the history survived, then assert the head role.
💚 Proposed fix
- if (engine.debugHistory.isNotEmpty) {
- expect(engine.debugHistory.first['role'], isNot('tool'));
- }
+ expect(engine.debugHistory, isNotEmpty);
+ expect(engine.debugHistory.first['role'], isNot('tool'));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (engine.debugHistory.isNotEmpty) { | |
| expect(engine.debugHistory.first['role'], isNot('tool')); | |
| } | |
| expect(engine.debugHistory, isNotEmpty); | |
| expect(engine.debugHistory.first['role'], isNot('tool')); |
🤖 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/coach_history_trim_test.dart` around lines 66 - 68, Update the
debugHistory assertions in the coach history test to first require that
engine.debugHistory is not empty, then unconditionally verify its first entry’s
role is not 'tool'. Remove the conditional wrapper so an empty history causes
the test to fail.
| import 'package:flutter_test/flutter_test.dart'; | ||
| import 'package:openstrap_analytics/onehz.dart' as ana; | ||
|
|
||
| /// Per-minute rows carrying DELIBERATELY MISLEADING ENMO: every sedentary | ||
| /// minute sits at 0.055 g, just above the absolute 0.05 g floor the old | ||
| /// estimator used. If an ENMO-based decision path is ever reintroduced these | ||
| /// tests fail loudly instead of shipping tens of thousands of phantom steps. | ||
| List<ana.MotionMinute> rows(List<double> dyn) => [ | ||
| for (var i = 0; i < dyn.length; i++) | ||
| ana.MotionMinute(i * 60000.0, 60, 0.055, 0.02, 1.055, dyn[i]), |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔴 Critical | ⚡ Quick win
Blocking: the entire file fails flutter analyze — openstrap_analytics is not repinned.
Every symbol this suite depends on is undefined in the currently pinned analytics revision: personalDynFloorFromDailySummaries, personalDynFloorMinDays, dailyDynSummary, the personalDynFloorG named parameter, DailyStepEstimate.activeMinutes, and MotionMinute's 6th positional argument (line 31: 5 expected, 6 given). This fails the whole test job, not just this file, so nothing else in the PR is being verified by CI.
Repin the openstrap_analytics dependency to the revision containing the personal-floor API before merging (the PR notes this is pending the analytics merge). If the merge is not imminent, gate the suite so CI stays green in the meantime.
🧰 Tools
🪛 GitHub Actions: test / 0_test.txt
[error] 31-31: flutter analyze: Too many positional arguments: 5 expected, but 6 found. (extra_positional_arguments)
🪛 GitHub Actions: test / test
[error] 31-31: Too many positional arguments: 5 expected, but 6 found when calling a function. (extra_positional_arguments)
🤖 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/step_personal_floor_test.dart` around lines 22 - 31, The test depends on
analytics APIs absent from the pinned openstrap_analytics revision. Repin
openstrap_analytics to the revision containing
personalDynFloorFromDailySummaries, personalDynFloorMinDays, dailyDynSummary,
personalDynFloorG, DailyStepEstimate.activeMinutes, and the six-argument
MotionMinute constructor; if unavailable immediately, gate or disable this suite
so flutter analyze and CI remain green.
Source: Pipeline failures
Edge CI was failing to COMPILE, and the failure is the whole reason the v48 note existed: error • The function 'personalDynFloorFromDailySummaries' isn't defined error • The named parameter 'personalDynFloorG' isn't defined error • The getter 'activeMinutes' isn't defined for DailyStepEstimate lib/compute/ calls analytics API that only exists on the sibling PR, but pubspec.yaml still pointed at the pre-fix SHA. It compiled locally only because the gitignored pubspec_overrides.yaml redirects to ../analytics — so the green local run was green-by-omission, and CI was the only thing telling the truth. This is the same drift class as v43, caught in its loud form: a missing symbol rather than a silently different number. The v43 version of this shipped a readiness fix that the pinned SHA never contained and stayed broken for three releases. Both pins now point at the sibling PR heads — still immutable commit SHAs, never floating refs: protocol 5675b2f (OpenStrap/protocol#19) analytics 8b1aa4e (OpenStrap/analytics#30) Verified, rather than assumed, that those SHAs contain what the changelog claims: `git show 8b1aa4e:lib/src/onehz/motion/steps.dart` carries the new step API, rr_correction.dart has the signed-dRR `seg.add(x[k])`, and `git show 5675b2f:lib/src/live.dart` has kKnownRecordVersions. Also corrected the kAlgoVersion changelog. It still said the sibling fixes were NOT pinned, which stopped being true with this commit — a changelog that misdescribes what is in the build is the exact failure being guarded against, in either direction. pubspec.lock now records the real git provenance (resolved-ref) instead of the local path: entries a previous `pub get` wrote while the overrides file was present. Reproduced CI locally to confirm: with pubspec_overrides.yaml moved aside, `flutter pub get` resolves ref == resolved-ref == 8b1aa4e, `flutter analyze` is clean, and the suite is 914 passing. ON MERGE: move both pins to the resulting main commits and bump kAlgoVersion again — a PR-branch SHA can become unreachable if the branch is deleted after a squash merge.
|
Persistent review updated to latest commit 9f53dd9 |
OpenStrap/protocol#19 and OpenStrap/analytics#30 are merged, so pubspec.yaml moves off the PR-branch heads v49 pointed at and onto the resulting main commits: protocol 5675b2f -> a98cd70 analytics 8b1aa4e -> f5ccae6 A PR-branch SHA stops being the canonical location of a change once it merges, and can be orphaned outright if the branch is later deleted. kAlgoVersion 49 -> 50 even though no edge source line changed alongside it. The constant identifies the code that PRODUCED a day_result, and that code includes the pinned siblings — a device holding v49 rows built against the PR-branch SHAs must re-derive rather than serve them as equivalent. Treating "same content, different commit" as beneath a bump is the assumption that lets a stale bundle outlive a dependency change. Verified at the merge commits themselves rather than inferred from the PRs being green — the check v43 skipped: analytics f5ccae6 steps.dart has the new step API (9 symbol hits) rr_correction.dart:233 is `seg.add(x[k])` (signed dRR) advanced_stager.dart has maxAccelCarryForwardSec protocol a98cd70 live.dart has kKnownRecordVersions Reproduced CI locally with pubspec_overrides.yaml moved aside: resolved-ref matches both pins exactly, flutter analyze clean, 914 passing.
|
Persistent review updated to latest commit b014c52 |
1 participant
User description
A full-tree audit surfaced defects clustered in three seams the 886-test suite never reached: failure paths, absent-vs-zero coercion, and guards modelled with regexes and blocklists. Every fix carries a regression test that fails against the old code.
P0 — BLE: the safe-trim invariant held by ordering, but not by errors
DrainController.commit()cleared its buffer, then swallowed the durable commit's exception and returnedvoid. The caller ACKed regardless, so the band trimmed flash for records that were never persisted — buffer cleared, transaction rolled back, cursor unadvanced, data gone permanently and silently. This is reachable:db.dart:853-860documents the production OOM inside this exact transaction (Crashlytics 0.9.13) and asserts "every row is durable before the caller echoes the HISTORY_END trim token, or none is".commit()now reports failure, re-buffers at the front of the queue, and the ACK is gated on aTrimAckPolicyverdict.Also:
_drainOffloadFrames/_handleSyncMarkerwere the only two async paths without the_session != sessionguard four siblings make, so a stale drainer could ACK an old token with a reused seq onto a new link.discardOpenChunknow poisons its burst so a straggler terminal can't trim discarded records. A failed connect no longer holds the band claim forever (which starved the headless drainer for the process lifetime), link-down tears the session down instead of leaking five timers and four subscriptions per drop, a corrupt far-future RTC is refused, and the Android boot wake goes through the headless gate.P0 — Persistence: three ways a good day could be destroyed
_markDaySkippedREPLACEd a realday_resultwith a skip marker after one 90 s isolate timeout, finalized and unrecoverable once raw was pruned 3 days later.endSec = 0, so it always looked finalizable and NULLed every scalar inmetric_series.appendScalars, so each finished day polluted the window the next one read — the same duplicate-day → MAD=0 shape edge#108 fixed on the load path.Plus:
exportDaysDbpassedonCreatewith noversion, so it threwArgumentErroron every call and had never worked; two unguardedALTER TABLE ADD COLUMNcollided with modernized CREATE helpers and permanently bricked upgrades from old schemas (rollback → rethrow → stuck on the loading screen every launch); the orphan guard missed the counter-PK eviction anddecodedRrByCounterRangereturned zero rows across a counter reset;deleteDaysnever cascadedworkout_route; and local days were assumed to be exactly 86400 s.P0 — Privacy
The coach SQL guard leaked GPS. Its regex ignored comma cross-joins and its blocklist was missing five real tables, so
SELECT r.lat, r.lng FROM v_sessions s, workout_route rpassed both gates and returned on-device-only coordinates into an LLM prompt bound for a third-party API. Replaced with a lexer + allow-list parser, plus a structural gate that derives the permitted btree root pages byEXPLAINing the coach views — so a table added tomorrow is closed by default. 49 adversarial tests, including that exact query.Firebase collected by default. No collection flags existed in the plist or manifest, and the only gate ran after an async prefs read inside fire-and-forget init — so a fresh install auto-logged
first_open/session_startand uploaded startup crashes before consent was read. Collection is now off at the platform level (verified in the merged manifest after a real build) and enabled only once consent resolves.High — absent is not zero
0on every day, on every device:_n(segments)coerced a List to null, then?? 0.TrendBoarddiscarded thehasflag the repository emits precisely to mark absent days, so unworn days drew real, tappable bars under an InfoDot promising "empty periods stay empty" — and the oxygen board printed a literal "0" for an unmeasured night.getDayWearcollapsed "never measured" intoworn_min: 0, so an imported day asserted "Not worn on this day".DeltaChipwas fed an absolute delta and rendered it as a percentage: sleeping 20 min more than last week displayed "▲ 20.0%".Charts now take nullable values and render gaps as gaps. Native widget/watch sentinels were kept in sync with the Dart writer.
High — state
Unpair could be silently undone by a late engine-state callback re-creating the pairing from a stale serial. The once-per-day notification guard was consumed before the notification was presented, so a recovery alert suppressed by quiet hours never fired and never retried. A denied notification permission was cached process-wide with no invalidation (zero notifications until an app restart), and schedules added absolute
Durations toTZDateTimeand drifted an hour across DST.Verification
flutter test --concurrency=1: 886 passing, 0 failing (was 623).flutter analyze: No issues found.flutter build apk --debug: builds; merged manifest carries the collection flags asfalse.Info.plistchange is declarative keys only, but it needs an iOS build on a machine with pods before release.kAlgoVersiongoes 47 → 48, and its changelog documents only the edge-local changes.The sibling analytics fixes from the same sweep are deliberately not claimed, because
pubspec.yamlstill pins the previous analytics SHA (e3705bf), which does not contain them. Citing an analytics change that the pinned SHA never contained is exactly how the v43 changelog documented a readiness fix that stayed broken for three releases.After OpenStrap/analytics#30 and OpenStrap/protocol#19 merge: repin both SHAs, verify with
git show <sha>:<file>that the pinned commit really contains the cited change, and bumpkAlgoVersionagain.Related: OpenStrap/protocol#19, OpenStrap/analytics#30
PR Type
Bug fix, Tests
Description
Closes P0 BLE data-loss:
DrainController.commit()now returnsbooland re-buffers on failure; ACK is gated byTrimAckPolicyso the band never trims flash for records that were never persistedFixes stale-session and band-claim leaks: failed/dropped connects now release the band claim via
_failConnect();_drainOffloadFramesand_handleSyncMarkergain session guards; link-down tears down timers/subscriptions immediatelyFixes DB migration ladder:
_addColumnIfMissingreplaces bareALTER TABLE … ADD COLUMN(preventing "duplicate column name" app-stuck-forever);exportDaysDbgains mandatoryversion:(it had never produced a file);deleteDaysnow cascadesworkout_route,cycle_symptom,workout_suggestions,sleep_override;decodedRrByCounterRangefixed for counter-reset pages;_localDayWindowuses DST-correct next-midnight instead of+86400Fixes absent-vs-zero coercions: wear/coverage, timeline events (now SQL-bounded by day), Records screen (payload-free SQL), cycle-screen
clampcrash on short cycles, notification permission cache invalidated on foreground; adds 30+ regression tests covering all changed pathsDiagram Walkthrough
File Walkthrough
5 files
Safe-trim invariant, session guards, band-claim leak fixesMigration ladder hardening, export fix, DST-correct day windowsAbsent-vs-zero fixes for wear, events, records, day windowsInvalidate notification permission cache on foreground resumeDST-correct localDayEndSec exported for shared use13 files
Regression tests for DrainController commit/re-buffer/ACK gateTests for session-staleness and band-claim release pathsTests for migration ladder, export version, deleteDays cascadeTests for _addColumnIfMissing idempotence across upgrade pathsTests for absent-vs-zero coercion in wear and coverage fieldsTests for events-in-range, records screen, cycle clamp crashTests for DST-correct local day window boundariesTests that real day_result is never overwritten by skip markerTests for derivation day-window DST correctnessTests for orphan guard on decoded_onehz import pathTests for permission cache invalidation on foregroundTests for notification fire-once day guard correctnessRegression tests for app_state flag-reset and latch bugs2 files
Centralized notification ID constantsShared async/mounted guard widgets for UI safety57 files
Summary by CodeRabbit