fix: apple health strength workouts, workout strain, steps, imports, android battery + reconnect - #209
Merged
Conversation
we were sending STRENGTH_TRAINING on both platforms, but that value only exists on Health Connect - HealthKit rejects it, so every strength workout was dropped on iOS and the error went to a debugPrint nobody sees. swims had the same problem the other way round (bare SWIMMING is iOS-only, so they were being dropped on android).
strain for a live session was accumulated in RAM by the foreground app, so anything the app slept through was missing from it. background the app for a long workout (or let iOS kill it) and you stop the workout with a strain built from the few minutes the app was awake for - usually a handful of sub-resting minutes, which score exactly 0.0 next to a perfectly real duration and HR. the band had the whole window at 1 Hz the entire time. sessions are now rescored from that once it drains in, taking whichever of the two is higher (both are lower bounds over the same window, so this only ever improves and it settles). runs on read and after each drain, so existing broken workouts fix themselves. also stopped the share cards printing '0.0 Strain' for a session that was never scored at all.
the day total already prefers the phone's count over the band's - they're the same walk seen from your pocket and your wrist - but the today tile and the steps screen then added the band's live count on top of it. also fixed the today tile still saying 'est'; nothing estimates steps any more.
…ut 0 steps turning the toggle on pulled the counts into the db but never re-derived, and every screen reads the derived scalars - so if the band wasn't connected you granted permission and the tile just kept showing a dash. turning it off already re-derived; now both do. live workout steps need the band's 100 Hz stream, which is often not up (standard-HR fallback, background downgrade). we were printing '0 STEPS' for that, next to a real distance and a real HR. shows a dash instead when nothing was measured.
both import buttons fed whatever you picked straight into a utf8 decoder. a WHOOP 'my data' export is a zip of csvs and a .noopbak is a zip around noop's sqlite db, so you got 'FormatException: Unexpected extension byte (at offset 10)' - offset 10 being the first byte of a zip header with its high bit set. now: WHOOP export zips are unpacked and imported, a .noopbak tells you to export the raw sensor csv instead, and a file we can't read says so instead of quoting a byte offset. an export we can read but don't recognise (wrong language, wrong file) now fails with the columns it found rather than cheerfully reporting 'imported 0 days'.
…fix reconnect giving up for good battery: we asked for CONNECTION_PRIORITY_HIGH once at connect and never stepped back down, so an ~11ms connection interval was held 24/7 on a link that's meant to stay up forever - all night, with nothing to say. it now follows what's actually happening: high during an offload or a live workout, balanced when idle, low power in the background. the app-state backfill timer also ran every 10 min through the 'manual' path, which is deliberately not rate-limited, so it bypassed the 15-min floor while backgrounded; it defers to the engine's floored timer there now. band battery is polled every 5 min instead of every 30s. reconnect: the retry loop only ever started on a connected->disconnected transition and the whole loop sat in one try/catch, so a single throw inside it killed reconnect for the life of the process - which on android is forever, because the foreground service keeps the process alive. that's the 'take the band off for 10 minutes and it never comes back unless you re-pair' report. each attempt is now contained, and a supervisor checks every minute that we're actually trying (and restarts a loop that's been wedged too long). the bond-refusal pause had the same shape: it was only cleared inside the createBond success path, which the pause itself prevents from running. it expires after 30 min now.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds BLE link-priority and reconnect supervision, substrate-based session rescoring, ZIP import support, platform-specific health mappings, and nullable step and strain handling. ChangesBLE connectivity management
Session score reconciliation
Archive import resolution
Health activity mapping
Measurement and display semantics
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 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/ble/ble_engine.dart`:
- Around line 624-648: Serialize all Android connection-priority changes by
routing _applyLinkPriority invocations through a single async queue, ensuring
each queued operation recomputes the target from current session, background,
offload, and live-consumer state before calling requestConnectionPriority.
Preserve retry behavior by updating _appliedPriority only after success, and add
coverage proving concurrent transitions are applied in request order with the
newest state winning.
In `@lib/data/local_repository_impl.dart`:
- Around line 2019-2025: Update _rescoreSessionFromSubstrate to return the
fetched heart-rate sample rows alongside its rescore result, then have
getWorkout reuse those rows for the enrichment block instead of calling
LocalDb.hrSamplesInRange again. Preserve the existing rescore and enrichment
behavior while ensuring each session range is queried only once.
- Around line 2471-2477: Update the lookback-bound calculation in the
sessions-in-range flow to use the existing localDayStartSec helper rather than
subtracting sinceDays * 86400 from nowSec. Preserve the current-day upper bound
and pass an absolute epoch-second range to LocalDb.sessionsInRange, with the
lower bound aligned to the local start of the requested lookback day.
In `@lib/health/health_export.dart`:
- Around line 1027-1028: Update the _activity() mapper to pass
HealthExporter.isApple rather than Platform.isIOS, ensuring macOS and iOS
workouts both use the Apple activity-type mapping while preserving the existing
type conversion.
In `@lib/import/import_container.dart`:
- Around line 195-201: Update the extraction flow around resolveImportCsvPaths
so temporary CSV files are always deleted after parsing. Prefer wrapping the
importer’s parse/use of the extracted paths in a try/finally that recursively
removes the created temp directory, or return a disposable extraction result
that guarantees equivalent cleanup; preserve successful parsing and ensure
cleanup also runs when parsing fails.
- Around line 119-203: Add regression tests covering
lib/import/import_container.dart lines 119-203 for container classification,
CSV-member filtering and extraction, .noopbak SQLite rejection, malformed and
empty archives, malformed UTF-8, and actionable errors; cover
lib/import/noop_import.dart lines 118-139 for raw-sensor CSV selection from ZIPs
and unsupported NOOP backups; and cover lib/import/whoop_import.dart lines
88-143 for ZIP resolution, unknown headers, and recognized-but-empty exports.
Use the existing importer test conventions and assert both outcomes and relevant
failure messages.
- Around line 197-200: Update the archive extraction loop around csvFiles so
destination filenames remain unique when members share the same basename,
preserving distinct files such as daily/data.csv and workouts/data.csv. Avoid
overwriting existing destinations and ensure out contains each extracted file’s
unique path exactly once.
- Around line 157-160: Update _extractCsvMembers() to decode and extract the
selected ZIP defensively off the UI isolate, enforcing limits on compressed
input size, member count, each member’s uncompressed size, and cumulative
uncompressed size before writing files. Prefer the streaming or recorded-entry
limit APIs supported by archive 4.0.9, and avoid unbounded f.content as
List<int> materialization while preserving CSV selection behavior.
In `@lib/import/whoop_import.dart`:
- Around line 96-107: Update the CSV processing flow around _readCsv and
_classify so a file with at least one row has its first row classified before
checking for data rows. Increment recognisedFiles for recognized headers even
when no data rows exist, and skip only after classification when rows contain no
data; preserve the existing unknown-header tracking behavior.
In `@lib/state/app_state.dart`:
- Around line 2502-2508: Update the reconnect flow around _reconnect and the
restartStale handler to invalidate the stalled loop before starting its
replacement, using a reconnect generation token or cancellable loop controller.
Ensure only the active generation may initiate connections, mutate
_reconnecting/_reconnectingSince, or perform finally cleanup, so the original
loop cannot affect the replacement. Add a regression test that releases the
original stalled attempt after restartStale and verifies no stale state mutation
or extra reconnect loop occurs.
- Around line 2037-2063: Update the workout sample-tracking state used by
workoutStepsMeasured so reconnecting and _resetLivePedometer() cannot invalidate
the indication that gait-capable samples were received during the active
workout; preserve the null result only when no such samples have ever arrived,
while continuing to calculate steps from the current live counters. Update the
relevant reset and sample-ingestion paths, and add a regression test covering
disconnect/reconnect during an active workout and ensuring stopWorkout() retains
the measured steps.
- Around line 2498-2502: Terminate the ReconnectSupervisorAction.start branch
after unawaited(_reconnect()) by adding a return or equivalent terminating
statement before the ReconnectSupervisorAction.restartStale case.
In `@lib/ui/screens/screens.dart`:
- Around line 280-288: Replace the direct phoneStepsEnabled gating in the
live-step selection of lib/ui/screens/screens.dart:280-288 with the shared
authoritative reactive source-selection value, preserving band live steps when
phone rows are unavailable. Apply the same source-selection value in
lib/ui/today/today_screen.dart:352-361 and include it in that view’s reactive
selection so tiles and week rings update when the authoritative source changes.
In `@test/session_score_reconcile_test.dart`:
- Around line 29-168: Add repository-level tests covering
_rescoreSessionFromSubstrate and rescoreRecentSessions: verify rows whose status
is not 'done' are skipped and remain unchanged, and verify completed rows are
persisted only when merged.changed is true, with repeated rescoring producing no
additional write. Use the existing repository and test-fixture helpers rather
than testing only reconcileSessionScore.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 314818f7-b743-4701-b17e-ba15d5a375a2
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
lib/ble/ble_engine.dartlib/compute/manual_session.dartlib/data/local_repository.dartlib/data/local_repository_impl.dartlib/health/health_export.dartlib/import/import_container.dartlib/import/noop_import.dartlib/import/whoop_import.dartlib/state/app_state.dartlib/sync/sync_policy.dartlib/ui/activity/live_session_screen.dartlib/ui/activity/workout_share_card.dartlib/ui/screens/screens.dartlib/ui/today/today_screen.dartlib/ui/workouts/workouts_screen.dartpubspec.yamltest/import_container_test.darttest/link_priority_policy_test.darttest/reconnect_supervisor_test.darttest/session_score_reconcile_test.darttest/workout_health_mapping_test.dart
Comment on lines
+119
to
+203
| Future<List<String>> resolveImportCsvPaths( | ||
| List<String> paths, { | ||
| required String flavor, | ||
| }) async { | ||
| final out = <String>[]; | ||
| for (final path in paths) { | ||
| final kind = await sniffFile(path); | ||
| switch (kind) { | ||
| case ImportContainer.text: | ||
| out.add(path); | ||
| case ImportContainer.zip: | ||
| out.addAll(await _extractCsvMembers(path, flavor: flavor)); | ||
| case ImportContainer.sqlite: | ||
| throw ImportFormatException( | ||
| '“${p.basename(path)}” is a database file, not a $flavor CSV ' | ||
| 'export. In NOOP, use Export → raw sensor CSV and pick the ' | ||
| '“noop-raw-sensors-….csv” file it writes.', | ||
| ); | ||
| case ImportContainer.gzip: | ||
| throw ImportFormatException( | ||
| '“${p.basename(path)}” is a gzip archive. Unzip it first and pick ' | ||
| 'the CSV inside.', | ||
| ); | ||
| case ImportContainer.binary: | ||
| throw ImportFormatException( | ||
| '“${p.basename(path)}” is not a text file, so there is nothing to ' | ||
| 'read as a $flavor CSV export.', | ||
| ); | ||
| } | ||
| } | ||
| return out; | ||
| } | ||
|
|
||
| Future<List<String>> _extractCsvMembers( | ||
| String path, { | ||
| required String flavor, | ||
| }) async { | ||
| final name = p.basename(path); | ||
| final Archive archive; | ||
| try { | ||
| archive = ZipDecoder().decodeBytes(await File(path).readAsBytes()); | ||
| } catch (e) { | ||
| throw ImportFormatException( | ||
| 'Could not read “$name” as an archive: $e', | ||
| ); | ||
| } | ||
|
|
||
| final csvFiles = [ | ||
| for (final f in archive.files) | ||
| if (f.isFile && _isCsvMember(f.name)) f, | ||
| ]; | ||
|
|
||
| if (csvFiles.isEmpty) { | ||
| // The `.noopbak` case, and the single most-reported one: an archive whose | ||
| // payload is a SQLite database. Name the file we actually want rather than | ||
| // failing on its bytes. | ||
| final hasDb = archive.files.any( | ||
| (f) => | ||
| f.isFile && | ||
| (f.name.toLowerCase().endsWith('.sqlite') || | ||
| f.name.toLowerCase().endsWith('.db')), | ||
| ); | ||
| if (hasDb) { | ||
| throw ImportFormatException( | ||
| '“$name” is a full NOOP backup — it holds NOOP\'s own database, which ' | ||
| 'we can\'t read. In NOOP, open Export and choose the raw 1 Hz sensor ' | ||
| 'CSV (“noop-raw-sensors-….csv”), then import that file here.', | ||
| ); | ||
| } | ||
| throw ImportFormatException( | ||
| '“$name” is an archive with no CSV files inside ' | ||
| '(${archive.files.length} entr${archive.files.length == 1 ? 'y' : 'ies'}). ' | ||
| 'Pick the $flavor CSV export instead.', | ||
| ); | ||
| } | ||
|
|
||
| final dir = await Directory.systemTemp.createTemp('openstrap_import_'); | ||
| final out = <String>[]; | ||
| for (final f in csvFiles) { | ||
| final dest = File(p.join(dir.path, p.basename(f.name))); | ||
| await dest.writeAsBytes(f.content as List<int>); | ||
| out.add(dest.path); | ||
| } | ||
| return out; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Add regression tests for the archive-import contract.
This change modifies both production import paths, but no changed test/**/*.dart coverage is included. Add tests for ZIP CSV extraction, .noopbak SQLite rejection, malformed archives, empty archives, malformed UTF-8, and importer-specific invalid or unrecognized exports.
lib/import/import_container.dart#L119-L203: test container classification, CSV-member filtering, extraction, and actionable failure messages.lib/import/noop_import.dart#L118-L139: test raw-sensor CSV selection from a ZIP and unsupported NOOP backup handling.lib/import/whoop_import.dart#L88-L143: test WHOOP ZIP resolution, unknown headers, and recognized-but-empty exports.
As per coding guidelines, “Behavior changes, especially regressions involving readiness, abstention, idempotence, synchronization, migrations, and lifecycle safety, must include regression tests.”
📍 Affects 3 files
lib/import/import_container.dart#L119-L203(this comment)lib/import/noop_import.dart#L118-L139lib/import/whoop_import.dart#L88-L143
🤖 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/import_container.dart` around lines 119 - 203, Add regression
tests covering lib/import/import_container.dart lines 119-203 for container
classification, CSV-member filtering and extraction, .noopbak SQLite rejection,
malformed and empty archives, malformed UTF-8, and actionable errors; cover
lib/import/noop_import.dart lines 118-139 for raw-sensor CSV selection from ZIPs
and unsupported NOOP backups; and cover lib/import/whoop_import.dart lines
88-143 for ZIP resolution, unknown headers, and recognized-but-empty exports.
Use the existing importer test conventions and assert both outcomes and relevant
failure messages.
Source: Coding guidelines
Contributor
PR Reviewer Guide 🔍(Review updated until commit c66d083)Here are some key observations to aid the review process:
|
Contributor
PR Code Suggestions ✨ |
- the workout step latch was compared against a counter that _resetLivePedometer zeroes on every reconnect, so a mid-workout reconnect left steps reading as unmeasured for the rest of the session. it's a flag now, set where samples land, and it survives the reset the same way the raw base already does. - the today tile and steps screen were gating live steps on the phone-steps TOGGLE, but the db only lets phone rows win when the phone actually has data for the day. with the toggle on and nothing recorded (read denied on ios, nothing writing to health connect) the band's live count was being dropped. both views use the same question the db asks now. - the supervisor could declare a loop wedged, start a replacement, and then have the original loop's finally clear the replacement's state on its way out - which would let a third loop start. loops carry a generation now. - the rescore sweep re-read every recent session's 1 Hz window on every drain, on the db isolate. bounded to the raw-retention horizon and skips windows a previous pass already covered, so an ordinary drain reads nothing. - getWorkout was scanning the same window twice: once to rescore, once to enrich. the rescore hands its rows back. - zip members sharing a basename overwrote each other; extracted temp files were never deleted; a zip is now refused if it declares an absurd member count or unpacked size. - a whoop csv with the right header but no rows counted as unrecognised, so an export of empty-but-valid files told you to re-download it in english. - lookback bound uses local midnight instead of n*86400. - workout activity mapping and stopWorkout/finish-card steps go through the same isApple / nullable-steps seams the rest of the file uses.
Contributor
|
Persistent review updated to latest commit 23d624f |
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 (1)
lib/state/app_state.dart (1)
3255-3314: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winStop superseded attempts before connection setup.
The generation check only controls the next
whileiteration. If a stale attempt resumes fromwaitForOsAutoConnect,Future.delayed,_ensureForegroundLease, orconnectToRemoteId, it still performs connection setup and post-connect side effects. It can reset the replacement loop’s live pedometer or reconfigure its streams.Check
generation == _reconnectGenerationafter every await that precedes a connection or setup action. Include the generation inkeepWaiting.Proposed fix
final generation = ++_reconnectGeneration; + bool isCurrent() => + generation == _reconnectGeneration && + _keepAlive && + !device.autoReconnectPaused; + BandOwnership.markForegroundIntent(true); @@ - while (_keepAlive && + while (isCurrent() && !engine.isConnected && - !device.autoReconnectPaused && - generation == _reconnectGeneration) { + generation == _reconnectGeneration) { @@ connected = await engine.waitForOsAutoConnect( paired!.remoteId, - keepWaiting: () => _keepAlive && !engine.isConnected, + keepWaiting: () => isCurrent() && !engine.isConnected, ); - if (connected && _keepAlive) { + if (!isCurrent()) break; + if (connected) { await _ensureForegroundLease(); + if (!isCurrent()) break; connected = await engine.connectToRemoteId(paired!.remoteId); } else { @@ await Future.delayed(engine.reconnectDelay(attempt)); - if (!_keepAlive) break; + if (!isCurrent()) break; await _ensureForegroundLease(); + if (!isCurrent()) break; connected = await engine.connectToRemoteId(paired!.remoteId); } + if (!isCurrent()) break; if (connected) {🤖 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 3255 - 3314, Update the reconnect loop around waitForOsAutoConnect, Future.delayed, _ensureForegroundLease, and connectToRemoteId to capture the attempt generation and abort immediately whenever generation != _reconnectGeneration after each preceding await. Include the same generation check in keepWaiting, and guard post-connect side effects such as stream setup, _resetLivePedometer, and backlog handling so superseded attempts cannot modify the replacement reconnect loop.
♻️ Duplicate comments (1)
lib/state/app_state.dart (1)
2549-2553: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winTerminate the
startcase.Line 2552 completes normally. Dart rejects a non-empty
switchcase that reaches the next case. Addreturn;afterunawaited(_reconnect());.#!/bin/bash set -euo pipefail if command -v dart >/dev/null 2>&1; then dart analyze lib/state/app_state.dart else sed -n '2546,2561p' lib/state/app_state.dart fi🤖 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 2549 - 2553, Terminate the ReconnectSupervisorAction.start branch in the switch by adding an explicit return immediately after unawaited(_reconnect()); in the surrounding reconnect supervisor logic, before the restartStale case.
🤖 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/ble/ble_engine.dart`:
- Around line 648-665: Update _applyLinkPriority() so the post-await assignment
to _appliedPriority occurs only when the captured session is still the current
connected _session. Recheck session identity and connection state after
requestConnectionPriority() completes, preventing stale completions from caching
priority after _teardownSession() has cleared it.
In `@lib/data/local_repository_impl.dart`:
- Around line 2502-2509: Update the sweep around _rescoreSessionFromSubstrate to
track processed completed-session fingerprints using session ID and end
timestamp, rather than skipping solely when endTs <= _rescoredThroughTs. Allow
sessions that transition from live to done to be rescored, and invalidate their
fingerprint whenever completion status or end timestamp changes.
In `@lib/import/import_container.dart`:
- Around line 256-271: Ensure extracted CSV filenames remain unique across all
selected ZIP files, not just within one _extractCsvMembers() call. Update
resolveImportCsvPaths() and the extraction flow to either use a separate
archive-specific subdirectory for each ZIP or share a destination-name registry
across calls, so duplicate archive members cannot overwrite files or produce
duplicate returned paths.
In `@lib/ui/activity/live_session_screen.dart`:
- Around line 846-851: Update the PR detection logic around `_prSteps` to use
the effective measured step value from persisted workout data `d['steps']`,
falling back to `s.steps` consistently with the build path, instead of relying
only on `s.steps`. Preserve the existing null, positive-step, and
1.5-step-difference checks.
---
Outside diff comments:
In `@lib/state/app_state.dart`:
- Around line 3255-3314: Update the reconnect loop around waitForOsAutoConnect,
Future.delayed, _ensureForegroundLease, and connectToRemoteId to capture the
attempt generation and abort immediately whenever generation !=
_reconnectGeneration after each preceding await. Include the same generation
check in keepWaiting, and guard post-connect side effects such as stream setup,
_resetLivePedometer, and backlog handling so superseded attempts cannot modify
the replacement reconnect loop.
---
Duplicate comments:
In `@lib/state/app_state.dart`:
- Around line 2549-2553: Terminate the ReconnectSupervisorAction.start branch in
the switch by adding an explicit return immediately after
unawaited(_reconnect()); in the surrounding reconnect supervisor logic, before
the restartStale case.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0b47e6ed-e738-427b-8126-60a0b5e7dcf7
📒 Files selected for processing (12)
lib/ble/ble_engine.dartlib/data/db.dartlib/data/local_repository_impl.dartlib/health/health_export.dartlib/import/import_container.dartlib/import/noop_import.dartlib/import/whoop_import.dartlib/state/app_state.dartlib/ui/activity/live_session_screen.dartlib/ui/screens/screens.dartlib/ui/today/today_screen.darttest/import_container_test.dart
Contributor
PR Code Suggestions ✨ |
- a link-priority reply that landed after teardown wrote its target back into the cache the teardown had just cleared, so the next connection skipped its own request. discarded if the session it was asked on is gone. - the rescore skip keyed on the window alone, so a session that was still live during an earlier sweep was skipped forever once the frontier passed its end. keys on id+window and only records a session once it's actually finished. - two selected archives each containing data.csv wrote to the same temp path. one subdirectory per archive. - step-record detection read the snapshot only; it uses the persisted count first, like the rest of that screen.
Contributor
|
Persistent review updated to latest commit b63b253 |
Contributor
PR Code Suggestions ✨No code suggestions found for the PR. |
Collaborator
Author
|
@CodeRabbit review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
lib/data/local_repository_impl.dart (3)
2464-2465: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not cache failed reconciliation attempts.
The helper returns
hrRows: nullafter an exception. Lines 2523-2527 still mark that session as rescored. If its window is behindrec_ts_hw, later sweeps skip it for the rest of the process lifetime. Keep failed rows eligible for retry.Proposed fix
- if (key != null && (r['status']?.toString() ?? '') == 'done') { + if (key != null && + after.hrRows != null && + (r['status']?.toString() ?? '') == 'done') { _rescoredSessions.add(key); }Also applies to: 2523-2527
🤖 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/local_repository_impl.dart` around lines 2464 - 2465, Update the reconciliation result handling so rows returned with hrRows: null from the catch path are not marked as rescored or cached by the session rescore flow. In the logic around the reconciliation helper and the session update, only advance rec_ts_hw or record successful reconciliation when hrRows is non-null; preserve failed rows for retry in later sweeps.
2451-2462: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftUse a conditional score-only update for reconciliation.
LocalDb.putSession(updated)replaces the whole row byid. Since this updates after readingrowand awaiting HR/resting-HR reads, a concurrent retime or finalize write can race and leave stalestart_ts,end_ts, orstatus. Re-checkid,start_ts,end_ts, andstatusbefore scoring; if the window/status changed, reconcile again and update only the score fields.🤖 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/local_repository_impl.dart` around lines 2451 - 2462, Update the reconciliation flow around the merged score fields and LocalDb.putSession so it re-reads and validates id, start_ts, end_ts, and status after the awaited HR/resting-HR reads and before scoring. If the window or status changed, reconcile again; otherwise persist only the score-related fields (strain, calories, max_hr, and zone_min_json) using a conditional update that cannot overwrite concurrent retime or finalize changes.
2493-2493: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAlign the public default window.
LocalRepositorydeclaresrescoreRecentSessions({int sinceDays = 7}), whileLocalRepositoryImploverrides it withsinceDays = 3. Dart uses the static receiver type for omitted optional parameters, so callers throughLocalRepositoryuse 7 days and directLocalRepositoryImplcallers use 3 days. Set one default in both declarations.🤖 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/local_repository_impl.dart` at line 2493, Align the optional sinceDays default in LocalRepositoryImpl.rescoreRecentSessions with the LocalRepository declaration by choosing one shared default value and applying it to both declarations, so omitted arguments behave consistently regardless of the receiver type.lib/ble/ble_engine.dart (2)
1396-1408: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winRecord the initial battery read in the polling timestamp.
lib/state/app_state.dartcallsawait engine.getBattery()immediately after connection.getBattery()does not update_lastBatteryPollAt, so the first 30-second keep-alive sends a second battery request.The current code also records
_lastBatteryPollAtbefore_send()completes. A failed write then suppresses retries for five minutes.Use one battery-poll helper for the initial and periodic reads. Set
_lastBatteryPollAtonly after_send()returns successfully. The initial read is inlib/state/app_state.dartLines [3130-3144].🤖 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 1396 - 1408, Update the battery-read flow around getBattery() and the keep-alive polling block to use one shared battery-poll helper. Ensure the initial read from app state records the poll timestamp, and assign _lastBatteryPollAt only after _send(Cmd.getBatteryLevel, const []) completes successfully so failed writes remain retryable.
607-616: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftKeep every Android priority request in one serialized path.
_applyLinkPriority()uses_priorityInFlightand_priorityRestale, butconnect()callsrequestConnectionPriority()directly at Line [1203]. A background, live-mode, or offload transition can start_applyLinkPriority()while this request is pending. The fixedhighresult can then overwrite the newer target and_appliedPriority.The stale-session branch at Lines [664-672] has a second gap. A concurrent caller can set
_priorityRestale = true, but the stale completion returns before the loop consumes that flag. The new session can keep the setup priority without applying its current target.Route the setup request through the same serialized helper. Re-evaluate the current session after a stale completion. Add tests for a transition during connect setup and for session replacement while a priority request is pending.
This is a residual form of the earlier priority-serialization finding. Verify the ordering semantics of
flutter_blue_plus1.36.8 forrequestConnectionPriority().#!/bin/bash set -euo pipefail rg -n -C 8 \ 'requestConnectionPriority|_applyLinkPriority|_priorityInFlight|_priorityRestale|_appliedPriority' \ lib/ble/ble_engine.dart testAlso applies to: 636-679, 1196-1206
🤖 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 607 - 616, The Android connection-priority setup in connect() must use the serialized _applyLinkPriority() path instead of calling requestConnectionPriority() directly, preserving the current desiredLinkPriority and _appliedPriority ordering. Update the stale-session handling in _applyLinkPriority() so a completion that observes _priorityRestale re-evaluates the current session and consumes the pending flag before returning. Add coverage for transitions during connect setup and session replacement while a priority request is pending.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@lib/ble/ble_engine.dart`:
- Around line 1396-1408: Update the battery-read flow around getBattery() and
the keep-alive polling block to use one shared battery-poll helper. Ensure the
initial read from app state records the poll timestamp, and assign
_lastBatteryPollAt only after _send(Cmd.getBatteryLevel, const []) completes
successfully so failed writes remain retryable.
- Around line 607-616: The Android connection-priority setup in connect() must
use the serialized _applyLinkPriority() path instead of calling
requestConnectionPriority() directly, preserving the current desiredLinkPriority
and _appliedPriority ordering. Update the stale-session handling in
_applyLinkPriority() so a completion that observes _priorityRestale re-evaluates
the current session and consumes the pending flag before returning. Add coverage
for transitions during connect setup and session replacement while a priority
request is pending.
In `@lib/data/local_repository_impl.dart`:
- Around line 2464-2465: Update the reconciliation result handling so rows
returned with hrRows: null from the catch path are not marked as rescored or
cached by the session rescore flow. In the logic around the reconciliation
helper and the session update, only advance rec_ts_hw or record successful
reconciliation when hrRows is non-null; preserve failed rows for retry in later
sweeps.
- Around line 2451-2462: Update the reconciliation flow around the merged score
fields and LocalDb.putSession so it re-reads and validates id, start_ts, end_ts,
and status after the awaited HR/resting-HR reads and before scoring. If the
window or status changed, reconcile again; otherwise persist only the
score-related fields (strain, calories, max_hr, and zone_min_json) using a
conditional update that cannot overwrite concurrent retime or finalize changes.
- Line 2493: Align the optional sinceDays default in
LocalRepositoryImpl.rescoreRecentSessions with the LocalRepository declaration
by choosing one shared default value and applying it to both declarations, so
omitted arguments behave consistently regardless of the receiver type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5821c596-24fe-4295-95a2-fec463736d3d
📒 Files selected for processing (5)
lib/ble/ble_engine.dartlib/data/local_repository_impl.dartlib/import/import_container.dartlib/ui/activity/live_session_screen.darttest/import_container_test.dart
- the sweep's public default was 7 days and the impl's was 3. dart takes the default from the static receiver type, and every caller holds the interface, so the 3-day bound i added was never the one running. - a session whose rescore THREW was still marked as scored, so a transient db error retired it for the rest of the process. only cache a real read. - the rescore wrote the whole row back after two awaits, which would revert a retime or a finalize that landed in that window. re-reads and bails if the row moved. - the connect-setup priority request bypassed the serialized path and could race the transitions; it goes through the same helper now, and a reply that arrives after teardown re-evaluates for the new session instead of just bailing. - battery: one poll path shared with getBattery (the connect-time read no longer gets duplicated 30s later by the keep-alive) and the timestamp only moves once the write has actually gone out.
Collaborator
Author
|
@CodeRabbit review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/data/local_repository_impl.dart (1)
2451-2476: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftSerialize persistence for concurrent rescoring.
At Line 2457, two rescore calls can both pass the timestamp and status check. If a drain adds substrate rows between their reads, one call can persist a higher merged score and the other can then replace it with its older, lower score at Line 2476. This can regress strain, calories, maximum HR, and zone minutes.
Make the re-read, merge against the current row, and write one serialized database transaction or conditional monotonic update. Add a regression test that interleaves
getWorkoutandrescoreRecentSessionsand verifies that no persisted score field decreases.As per coding guidelines: "Behavior changes, especially regressions involving readiness, abstention, idempotence, synchronization, migrations, and lifecycle safety, must include regression tests."
🤖 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/local_repository_impl.dart` around lines 2451 - 2476, The rescore persistence path around LocalDb.session and LocalDb.putSession is vulnerable to concurrent writes regressing scores. Serialize the re-read, merge, and write in one database transaction, or use a conditional monotonic update that never decreases strain, calories, max HR, or zone minutes; add a regression test interleaving getWorkout with rescoreRecentSessions and verify no persisted score field decreases.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/ble/ble_engine.dart`:
- Around line 1418-1427: Update _pollBatteryIfDue to capture the boolean result
from _send(Cmd.getBatteryLevel, const []) and assign _lastBatteryPollAt only
when the write succeeds. Leave the existing due-check behavior unchanged so
failed writes remain eligible for the next scheduled poll.
In `@test/link_priority_policy_test.dart`:
- Around line 86-98: Replace the isolated desiredLinkPriority test with an
engine-level regression that starts background connection setup, drives the
_doConnect() path, and verifies that _applyLinkPriority() receives
ConnectionPriority.high. Ensure the test exercises the _connectSetup state
rather than merely passing offloadActive: true, and preserves the existing setup
lifecycle behavior.
---
Outside diff comments:
In `@lib/data/local_repository_impl.dart`:
- Around line 2451-2476: The rescore persistence path around LocalDb.session and
LocalDb.putSession is vulnerable to concurrent writes regressing scores.
Serialize the re-read, merge, and write in one database transaction, or use a
conditional monotonic update that never decreases strain, calories, max HR, or
zone minutes; add a regression test interleaving getWorkout with
rescoreRecentSessions and verify no persisted score field decreases.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fd510a4f-53e0-463c-9edc-e11a2a41c93e
📒 Files selected for processing (4)
lib/ble/ble_engine.dartlib/data/local_repository.dartlib/data/local_repository_impl.darttest/link_priority_policy_test.dart
Contributor
|
Persistent review updated to latest commit 58005b1 |
Contributor
PR Code Suggestions ✨ |
- the battery poll stamped its timestamp even when the write failed (_send reports failure as a return value, and i was ignoring it), so a dropped write silenced the next five minutes of polling. - my connect-setup test was vacuous: it re-asserted the pure rule with offloadActive: true, which the exhaustive offload case above it already covers, and would have passed with the wiring deleted. replaced with one that drives the engine's own state - checked it fails if the _connectSetup term is removed. _doConnect itself still can't run on the test host.
Contributor
|
Persistent review updated to latest commit 98bee1d |
Contributor
PR Code Suggestions ✨ |
reviewed the whole diff again since the bots stopped running (rate limited), and found real problems in my own fixes: reconnect (the #208 work): - the supervisor called a healthy loop wedged. staleness was measured from the LOOP's start with a 20 min threshold, but a single android autoConnect pass legitimately waits 15 min, so any band away longer than ~20 min got its loop torn down mid-attempt - and the abandoned attempt's disconnect() then cancelled the OS pending connect its replacement was waiting on. that's the never-reconnects symptom, caused by the thing meant to cure it. measured per-attempt now, threshold 25 min. - the supervisor could also start a loop underneath a user-initiated connect, which on android can outlast a 60s tick (bond dialog). it stands down while one is in flight. - supervisor kept ticking after unpair/endSession. battery + link priority (the #200 work): - 5-minute battery polling removed the only inbound traffic a quiet link has: _lastRx only advances on a notification, LINK_VALID is a write, so a link with no live stream would have crossed the 120s liveness fuse and been bounced every couple of minutes. it now forces a poll as silence approaches the fuse, keeping the power win when streams are flowing. - backgrounding skipped the ONLY periodic re-plan of the high-frequency wake window, so a band connected at 22:00 and left connected never armed high-freq sync for that night. the skip now covers the offload only. - a priority reply landing during teardown could restore the value teardown had just cleared, so the next connection thought it had already asked and never requested an interval. link generation check. - _connectSetup wasn't cleared when a connect failed before INIT, pinning the target at high for the life of the process. - an expired bond-refusal pause left 're-pair required' on screen with auto-reconnect silently re-armed behind it. session rescore (the #206 work): - the skip-set was stamped without the frontier check the skip itself uses, so a session scored while the band had handed over only part of its window was marked settled and the drain carrying the REST of it skipped the session entirely. that defeated the whole point of the sweep. - max-merging every re-score was only sound for a fixed scoring function. strain depends on the trailing nightly resting HR, which moves, so a session ratcheted up to whatever the most favourable RHR ever produced. once the substrate covers the window it now replaces the tally outright. - persisted max_hr was the raw 1 Hz peak, writing a PPG spike into the column getWorkout deliberately refuses to trust - and after the 3-day prune there'd be no smoothed value left to prefer. stores the smoothed peak. - the write was a whole-row REPLACE, which reverted hrr_bpm and type changes that land through their own narrow updates. score columns only now. - the concurrency bail handed back HR rows for the OLD window, which getWorkout then used to enrich the new one. import: - zip extraction buffered the whole archive and each member in memory, on the exact hundreds-of-megabytes export the size ceiling was written for. streams through InputFileStream/OutputFileStream now. - an archive with several CSVs silently imported one and reported success. - a UTF-16 CSV was called 'not a text file'. steps: - the phone-steps cache had no day key, so past midnight it kept yesterday's answer and suppressed the band's live count all day. keyed by day, refreshed on derive and on foreground return. - the re-derive after enabling phone steps was dropped whenever another derive was already running. - today's 'measured' tag rendered in warning amber.
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/import/noop_import.dart (1)
336-353: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject rows that contain no usable NOOP samples.
totalRowsincrements for every row with a parseableunix_svalue before the code validatesstreamor a measurement value. An unrelated CSV with timestamp-like rows and only unknown or unusable streams bypasses this error path, finalizes successfully, and imports no data.Track accepted supported samples separately. Use that count for this validation. Count valid
stepssamples as accepted so step-only exports remain valid. Add a regression test for timestamped rows with no supported samples.🤖 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/noop_import.dart` around lines 336 - 353, Update the import logic around totalRows so it separately counts accepted supported samples after validating stream and measurement values, then use that accepted count for the zero-row failure check. Ensure valid steps samples increment the accepted count so step-only exports remain valid, and add a regression test covering timestamped rows with no supported samples.lib/ble/ble_engine.dart (1)
679-683: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse the shared priority-target helper.
Line 679 duplicates the state-to-priority mapping in
linkPriorityForCurrentState(). Use that helper here so tests and radio requests use one policy path.Proposed fix
- final want = desiredLinkPriority( - offloadActive: _offloadActive || _connectSetup, - background: _backgrounded, - hasLiveConsumer: _liveEnabled && !_liveHrOnly, - ); + final want = linkPriorityForCurrentState();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/ble/ble_engine.dart` around lines 679 - 683, Replace the direct desiredLinkPriority call in the surrounding BLE engine flow with the existing linkPriorityForCurrentState() helper, passing or using the same current state inputs it requires. Remove the duplicated offload, connection-setup, background, and live-consumer priority mapping while preserving the resulting priority used by tests and radio requests.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/manual_session.dart`:
- Around line 528-533: The complete-coverage zone selection must not replace
existing values with an empty substrate vector. In
lib/compute/manual_session.dart lines 528-533, update the zone replacement
condition to require substrate.zoneMinutes.isNotEmpty; in
test/session_score_reconcile_test.dart lines 181-193, extend the completeness
regression test with an empty substrate zone and assert r.zoneMinutes remains
[1, 2, 0, 0, 0].
- Around line 509-526: Replace the duplicated maxHr nested-ternary logic with a
reusable generic merge helper based on better’s complete-versus-partial rule,
allowing both double? and int? values to use the same definition. Update the
strain, calories, and maxHr assignments to call that helper while preserving
substrate precedence when complete and larger-value selection when partial.
In `@lib/data/local_repository_impl.dart`:
- Around line 2577-2582: Update the change-count condition in the surrounding
method to count any row with after.hrRows != null whose rescore fields—strain,
calories, max_hr, or zone_min_json—differ from before. Ensure updates to
calories, max_hr, or zones increment changed even when strain is unchanged,
while preserving the existing before/after comparison behavior.
In `@lib/state/app_state.dart`:
- Around line 2558-2564: Update _stopReconnectSupervisor() to invalidate all
in-flight reconnect work: increment _reconnectGeneration, reset _reconnecting
and _attemptStartedAt, and clear the engine reconnect phase in addition to
cancelling the supervisor. Add a regression test covering endSession followed by
immediate openSession while reconnect is blocked or pending, ensuring the old
generation cannot resume as the active reconnect loop.
---
Outside diff comments:
In `@lib/ble/ble_engine.dart`:
- Around line 679-683: Replace the direct desiredLinkPriority call in the
surrounding BLE engine flow with the existing linkPriorityForCurrentState()
helper, passing or using the same current state inputs it requires. Remove the
duplicated offload, connection-setup, background, and live-consumer priority
mapping while preserving the resulting priority used by tests and radio
requests.
In `@lib/import/noop_import.dart`:
- Around line 336-353: Update the import logic around totalRows so it separately
counts accepted supported samples after validating stream and measurement
values, then use that accepted count for the zero-row failure check. Ensure
valid steps samples increment the accepted count so step-only exports remain
valid, and add a regression test covering timestamped rows with no supported
samples.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7d75b0d1-97da-4579-9572-5b6d269ec55b
📒 Files selected for processing (13)
lib/ble/ble_engine.dartlib/compute/manual_session.dartlib/data/db.dartlib/data/local_repository_impl.dartlib/import/import_container.dartlib/import/noop_import.dartlib/state/app_state.dartlib/sync/sync_policy.dartlib/ui/today/today_screen.dartlib/ui/workouts/workouts_screen.darttest/link_priority_policy_test.darttest/reconnect_supervisor_test.darttest/session_score_reconcile_test.dart
Contributor
|
Persistent review updated to latest commit cfbff28 |
- stopping supervision only cancelled the timer; a _reconnect() parked in waitForOsAutoConnect for up to 15 min kept its generation, so endSession followed by a fresh openSession let that zombie wake up and become the live loop. retires it by generation. - complete coverage wiped a stored zone split when the substrate's own vector was empty (which happens when the profile has no HRmax), while the scalar rule correctly fell back. same rule for both now, and one generic helper instead of the scalar rule written out twice. - the sweep's changed-count only looked at strain, so a pass that fixed calories or the zone split reported nothing.
Contributor
PR Code Suggestions ✨ |
Contributor
|
Persistent review updated to latest commit c66d083 |
Contributor
PR Code Suggestions ✨ |
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/state/app_state.dart (2)
2583-2593: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDetect the background startup connection as in flight.
busyis false during the background initialization path. That path starts_startReconnectSupervisor()before it awaitsengine.connectToRemoteId(). If setup lasts longer than one supervisor tick, this call returnsstartand begins a second reconnect loop.Pass the engine connection state as
connectInFlight, or add an explicit AppState connection-attempt flag. Add a regression for a background startup connect that remains in progress across a supervisor tick.Proposed fix
- connectInFlight: busy, + connectInFlight: + busy || (engine.holdsBandLink && !engine.isConnected),🤖 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 2583 - 2593, Update the superviseReconnect call in AppState to report the background engine connection as in flight by incorporating engine’s active connection state into connectInFlight, or use an explicit AppState attempt flag covering engine.connectToRemoteId(). Preserve busy-based detection for existing paths and add a regression test where startup connection remains pending across a supervisor tick without starting a second reconnect loop.
3289-3337: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftStop superseded reconnect attempts after every await.
The loop checks
generationonly at the nextwhileiteration. A stale loop can return fromwaitForOsAutoConnect()or the retry delay and still callengine.connectToRemoteId(). Its OS pending connect also remains armed becausekeepWaitingignores the generation.Include
generation == _reconnectGenerationinkeepWaiting. Re-check it after each await and before every connection or post-connect action. Add a regression that releases an old stale OS-connect attempt afterrestartStale.🤖 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 3289 - 3337, Update the reconnect loop around the visible OS fallback and direct-connect awaits to treat generation changes as cancellation: include generation == _reconnectGeneration in waitForOsAutoConnect keepWaiting, re-check _keepAlive and generation after every await, and gate engine.connectToRemoteId plus all subsequent post-connect setup on that condition. Add a regression covering restartStale that releases the superseded OS-connect attempt.Source: Coding guidelines
♻️ Duplicate comments (1)
lib/state/app_state.dart (1)
2594-2607: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winTerminate the
ReconnectSupervisorAction.startcase.The
startcase completes normally beforerestartStale. Dart does not permit this switch fall-through. Addreturn;afterunawaited(_reconnect()).Proposed fix
case ReconnectSupervisorAction.start: _log('[RECONNECT] supervisor: disconnected with no loop running — ' 'starting one.'); unawaited(_reconnect()); + return; case ReconnectSupervisorAction.restartStale:Dart language documentation: must a non-empty `switch` case terminate with return, break, throw, or continue?🤖 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 2594 - 2607, In the reconnect supervisor switch, update the ReconnectSupervisorAction.start case to return immediately after unawaited(_reconnect()), preventing execution from continuing into ReconnectSupervisorAction.restartStale. Leave the restartStale behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/link_priority_policy_test.dart`:
- Around line 96-143: The wiring tests bypass the production transition by
calling debugBeginConnectSetup directly, so they do not verify that _doConnect
sets _connectSetup. Add a BLE transport seam or fake device, execute the
_doConnect connection-setup path, and assert that _applyLinkPriority requests
ConnectionPriority.high; retain the existing setup and sendInit coverage.
---
Outside diff comments:
In `@lib/state/app_state.dart`:
- Around line 2583-2593: Update the superviseReconnect call in AppState to
report the background engine connection as in flight by incorporating engine’s
active connection state into connectInFlight, or use an explicit AppState
attempt flag covering engine.connectToRemoteId(). Preserve busy-based detection
for existing paths and add a regression test where startup connection remains
pending across a supervisor tick without starting a second reconnect loop.
- Around line 3289-3337: Update the reconnect loop around the visible OS
fallback and direct-connect awaits to treat generation changes as cancellation:
include generation == _reconnectGeneration in waitForOsAutoConnect keepWaiting,
re-check _keepAlive and generation after every await, and gate
engine.connectToRemoteId plus all subsequent post-connect setup on that
condition. Add a regression covering restartStale that releases the superseded
OS-connect attempt.
---
Duplicate comments:
In `@lib/state/app_state.dart`:
- Around line 2594-2607: In the reconnect supervisor switch, update the
ReconnectSupervisorAction.start case to return immediately after
unawaited(_reconnect()), preventing execution from continuing into
ReconnectSupervisorAction.restartStale. Leave the restartStale behavior
unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4db17f06-8ee7-42bc-a5e9-d172965a297c
📒 Files selected for processing (13)
lib/ble/ble_engine.dartlib/compute/manual_session.dartlib/data/db.dartlib/data/local_repository_impl.dartlib/import/import_container.dartlib/import/noop_import.dartlib/state/app_state.dartlib/sync/sync_policy.dartlib/ui/today/today_screen.dartlib/ui/workouts/workouts_screen.darttest/link_priority_policy_test.darttest/reconnect_supervisor_test.darttest/session_score_reconcile_test.dart
Comment on lines
+96
to
+143
| group('the engine feeds its own state into that rule', () { | ||
| // The policy tests above prove the RULE. These prove the WIRING, which is | ||
| // where the bug actually was: the connect-setup boost used to be a direct | ||
| // radio call that bypassed the serialized path entirely. | ||
| // | ||
| // Honest limit: `_doConnect` cannot run on the test host (flutter_blue_plus | ||
| // is unsupported there), so what is covered is the flag's effect on the | ||
| // target and `sendInit`'s clearing of it — not the assignment inside | ||
| // `_doConnect` itself. | ||
| late BleEngine engine; | ||
|
|
||
| setUp(() { | ||
| TestWidgetsFlutterBinding.ensureInitialized(); | ||
| engine = BleEngine( | ||
| onRecord: (sample, raw) async {}, | ||
| onState: (_) {}, | ||
| log: (_) {}, | ||
| ); | ||
| }); | ||
|
|
||
| test('a backgrounded idle engine wants the cheap interval', () { | ||
| engine.setBackground(true); | ||
| expect(engine.linkPriorityForCurrentState(), LinkPriority.lowPower); | ||
| }); | ||
|
|
||
| test('connect setup outranks being backgrounded', () { | ||
| engine.setBackground(true); | ||
| engine.debugBeginConnectSetup(); | ||
| expect( | ||
| engine.linkPriorityForCurrentState(), | ||
| LinkPriority.high, | ||
| reason: 'setup is immediately followed by the first flash drain', | ||
| ); | ||
| }); | ||
|
|
||
| test('sendInit ends the setup boost', () async { | ||
| engine.setBackground(true); | ||
| engine.debugBeginConnectSetup(); | ||
| // No session on the host, so the writes fail — the point is that the | ||
| // flag is cleared in a `finally`, not only on the happy path. | ||
| await engine.sendInit(); | ||
| expect( | ||
| engine.linkPriorityForCurrentState(), | ||
| LinkPriority.lowPower, | ||
| reason: 'an idle background link must stop paying for setup speed', | ||
| ); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Test the production connection-setup transition.
debugBeginConnectSetup() sets the state under test directly. The tests do not execute the _doConnect() path that sets _connectSetup = true. A regression that removes that production assignment still passes.
Inject a BLE transport seam or fake device. Execute connection setup and verify that _applyLinkPriority() requests ConnectionPriority.high.
As per coding guidelines, “Behavior changes, especially regressions involving readiness, abstention, idempotence, synchronization, migrations, and lifecycle safety, must include regression tests.”
🤖 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/link_priority_policy_test.dart` around lines 96 - 143, The wiring tests
bypass the production transition by calling debugBeginConnectSetup directly, so
they do not verify that _doConnect sets _connectSetup. Add a BLE transport seam
or fake device, execute the _doConnect connection-setup path, and assert that
_applyLinkPriority requests ConnectionPriority.high; retain the existing setup
and sendInit coverage.
Source: Coding guidelines
Collaborator
Author
|
Fair, and it's the limit I noted in the test itself. Not doing it here though.
What is covered: the flag's effect on the target and |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes a batch of open issues.
Strength workouts never showed up in Apple Health (#184). I was sending
STRENGTH_TRAININGon both platforms, but HealthKit doesn't have that value — it rejected every one and the error went to adebugPrint. Swims had the mirror-image bug on Android.Strain 0.0 after a long workout (#206). Strain was accumulated in memory by the foreground app, so whatever the app slept through simply wasn't in it — backgrounded long enough (or killed by iOS), the workout ended with a score built from the few minutes the app was awake for. The band had the whole thing at 1 Hz anyway, so I rescore sessions from that once it syncs, on read and after each sync. Old workouts fix themselves. Share cards no longer print
0.0for a session that was never scored.Steps (#183). With phone steps on, the today tile and steps screen added the band's live count on top of the phone's count of the same walk. The toggle also didn't re-derive when switched on, so with the band disconnected it granted permission and kept showing a dash. And a live workout showed "0 STEPS" whenever the band's 100 Hz stream wasn't up — a dash now, since nothing was measured.
Both imports failing with a FormatException (#199, #160). A WHOOP export is a zip of CSVs and a
.noopbakis a zip around noop's sqlite db, and I was feeding both straight into a UTF-8 decoder — hence "Unexpected extension byte (at offset 10)". WHOOP zips are now unpacked and imported, a.noopbakgets a message pointing at the raw sensor CSV, and a file I can't read explains why. An export I can read but don't recognise (e.g. downloaded in another language) now fails with the columns it found instead of reporting "imported 0 days".Background battery drain on Android (#200). I requested
CONNECTION_PRIORITY_HIGHonce at connect and never lowered it, so an ~11 ms connection interval was held around the clock on a link that's meant to stay up permanently. It now tracks what's happening: high during a sync or a live workout, balanced when idle, low power in the background. Separately, the 10-minute backfill went through the un-rate-limited "manual" path and so ignored the 15-minute floor while backgrounded; it leaves that to the engine's floored timer now. Band battery is polled every 5 minutes rather than every 30 seconds.Band never reconnects after being off the wrist or out of range (#208). The retry loop only started on a connected → disconnected transition, and the whole loop was inside one try/catch — so one throw anywhere in it ended reconnect for the life of the process, which on Android is forever because the foreground service keeps the process alive. Attempts are contained individually now, and a supervisor checks every minute that a reconnect is actually being attempted. The bond-refusal pause had the same shape (only cleared inside the bond-success path that the pause prevents from running) and now expires after 30 minutes.
Not fixed here: reading a
.noopbak's database. The importer points at the raw sensor CSV instead.Summary by CodeRabbit
New Features
Bug Fixes