Repository navigation
feat(polar): add Polar H10 concurrent BLE telemetry, 3D torso biomechanics, and HRR analytics - #1326
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: 9 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
VerdictRequest changes - the HRR-300 erasure is silent data loss in stored recovery data and the failed-STOP path kills ECG capture for the rest of the session; both should land before this ships. Correctness / Safety Findings
Note: the inline comment at Ponytail Review
Ponytail net: -45 lines. Suggested Minimal PatchSix files, no new dependencies:
Final Merge GuidanceDo not merge until the HRR-300 erasure and the failed-STOP state leak are fixed; the remaining warnings can land in the same patch. The Ponytail suggestions are optional cleanup. Note: PR #1326 was already merged while this review was running, so these findings need a follow-up patch on the merged branch rather than a re-review. Fix these issues in Kilo Cloud Files Reviewed (38 files)
Previous Review Summaries (4 snapshots, latest commit afbbcba)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit afbbcba)Status: 2 Issues Found | Recommendation: Merge Overview
Issue Details (click to expand)SUGGESTION
VerdictApprove - every correctness finding from the previous round is fixed in this increment; only optional Ponytail cleanup remains. Correctness / Safety FindingsNo correctness or safety findings. Ponytail Review
Ponytail net: -7 lines. Suggested Minimal PatchTwo files, no new dependencies:
Final Merge GuidanceCan merge as-is; the Ponytail suggestions are optional cleanup. Fix these issues in Kilo Cloud Files Reviewed (5 files)
Previous review (commit 50873a8)Status: 7 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
VerdictComment only - the incremental diff fixes the reconnect-job race, the contact diagnostic and the Koin default, but two narrow gaps remain: the manual-stop path still drops the Correctness / Safety Findings
Ponytail Review
Ponytail net: -73 lines. Suggested Minimal PatchThree files, no new dependencies:
Final Merge GuidanceCan merge after the two WARNING findings are fixed; the Ponytail suggestions are optional cleanup. Fix these issues in Kilo Cloud Files Reviewed (6 files)
Previous review (commit 728f957)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
VerdictComment only — Both previously reported CRITICALs are fixed (the reconnect no longer cancels its own job, and the analytics card now receives a Ponytail Review
Ponytail net: -17 lines. Suggested Minimal PatchFour local changes, no new dependencies, no new files:
Final Merge GuidanceCan merge; the two warnings are worth fixing but neither blocks — the reconnect-tracking race is narrow (it needs the Files Reviewed (20 files)
Fix these issues in Kilo Cloud Previous review (commit 20552c2)Status: 13 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Ponytail Review
[Snapshot truncated.] Additional previous summary content was truncated to keep this comment within platform limits. Reviewed by minimax-m3 · Input: 272.3K · Output: 61.9K · Cached: 3.9M Review guidance: REVIEW.md from base branch |
|
Addressed all 15 review findings in 9015302:
|
There was a problem hiding this comment.
Review: Polar H10 telemetry (head 9015302)
Verdict: request changes
This PR cannot merge as is. CI is red on the head commit with 17 shared-suite failures, all introduced here. Two code paths overwrite or misattribute stored workout data, and the schema change edits a migration that already shipped in v1.0.7. The Kilo bot approved this same head after its own 16 findings were fixed, but it missed everything below, so treat that approval as noise.
CI failures on the head commit, from the Unit Tests job:
| Test class | Failures | Cause |
|---|---|---|
DesignSystemRatchetTest |
2 | 23 new RoundedCornerShape(N.dp) and 7 new Color.White-style literals push the counts over the ratchet ceilings |
RoutineCompleteScreenRuntimeReachabilityTest, RoutineCompleteRouteExitRuntimeTest |
12 | RoutineCompleteScreen now renders PolarSessionAnalyticsCard, which calls koinInject(); these tests render without Koin |
RoutineCompleteFullShellNavigationTest, RoutineCompleteEnhancedShellChromeTest |
3 | Their Koin modules have no PolarH10BleManager definition |
Local verification on the head commit: SchemaParityTest and the three Polar test classes pass, while RoutineCompleteFullShellNavigationTest and DesignSystemRatchetTest fail exactly as in CI. A throwaway probe test against PolarH10BleManager reproduced findings 1, 2, 5 and 6 below:
| Probe | Observed |
|---|---|
| Start Cooldown on an old session, then 61 s of beats at 70 bpm | Stored summary avg 150 / max 170 / min 120 / HRR-300 50 / pitch 40 became 70 / 70 / 70 / null / null |
| Let a cooldown complete, then ask for an unrelated session's summary | Returned the finished workout's numbers under the unrelated id; activeCooldownSessionId still held the old id |
| Cancel Cooldown, then send 5 more beats | All 5 were inserted into the finished session with phase POST_WORKOUT_COOLDOWN |
Findings
Critical, must fix before merge.
-
PolarSessionAnalyticsCard.kt:572withPolarH10BleManager.kt:381. Pressing "Start Cooldown" on a history card overwrites that old session's stored heart-rate summary. Whenever a strap is connected the card renders on every expanded history session, because the early return at line 162 is skipped, and the cooldown section at line 363 shows the button. The button callstriggerCooldown(oldSessionId).endSessionRecordingadopts that id as the active recording, and at 60 secondspersistPolarSummaryat line 514 writes a summary computed from the in-memory cooldown beats over the old row. Average, peak and minimum HR become resting values, and HRR-300 and the torso maxima become null. Fix: passshowCooldown = falsefrom History, and make the manager refuseendSessionRecordingandgetSessionSummaryfor any id other than the session it actually recorded. -
PolarH10BleManager.kt:485.getSessionSummary(id)only checks that a different session is not active. Once a cooldown completes,activeSessionIdis cleared at line 472 butsessionHrs,hrr60,hrr300and the torso maxima are not. From then on the method returns the last workout's numbers for any id.PolarSessionAnalyticsCard.kt:101falls back to exactly this call for history sessions that have no Polar columns, so expanding an old session shows the previous workout's HR, HRV and HRR as if they belonged to it. Same root cause as finding 1: record which session the in-memory data belongs to and return null for anything else. -
migrations/58.sqm:18. The Polar DDL was appended to migration 58, which already shipped in v1.0.7 and TestFlight build 2026102094 with the custom-equipment change. Devices at schema version 59 never run the new statements throughSchema.migrate, and theMigrationStatementsentry for 58 never runs for them either. They only get the tables and columns becausereconcileFullSchemaruns on every open on both platforms, so the safety net has become the primary upgrade path. The new parity test atSchemaParityTest.kt:1306models a database still at 58, which is the one case real devices are not in. Fix per CLAUDE.md: revert58.sqmand the 58 entry, add59.sqmandMigrationStatements[59], keep the manifest entries, and retarget the test to the 59 to 60 upgrade. -
CI is red, see the table above. For the Koin failures, inject the manager from
MainViewModel, which already holds it, intoSetSummaryCard,RoutineCompleteScreen,WorkoutTabandHistoryTabas a nullable parameter, and drop thekoinInject()defaults atSetSummaryCard.kt:111and inPolarSessionAnalyticsCard. Registering a stub in four test modules also works but spreads the dependency further. For the ratchet, useMaterialTheme.shapesand colour-scheme tokens in the three new components, or raise the ceilings deliberately in the same PR.
Important, should fix before merge.
-
PolarH10BleManager.kt:354and:334. "Cancel Cooldown", strap removal and disconnect end the cooldown but leaveactiveSessionIdset. For a single-exercise workout the state stays Completed until the user acts, so beats and ECG segments keep being inserted into the finished session with phase POST_WORKOUT_COOLDOWN for as long as the strap is worn. Clear the recording whenever a cooldown ends for any reason, and stop the PMD streams at that point. -
PolarH10BleManager.kt:397withActiveWorkoutScreen.kt:310._activeCooldownSessionIdis set when a cooldown starts and only cleared when the next recording starts. The navigation gate treats a non-null id as a running cooldown, so after the first cooldown ever runs, a Completed workout never auto-navigates back again. Decide the intended behaviour, then clear the id when the cooldown ends. -
PolarH10BleManager.kt:1029and:1170. After the set commits, every beat and every ECG packet becomes its own autocommit insert from the BLE dispatcher, two to four per second, concurrently with the repository's commit transaction. Failures are logged and dropped. The app already buffersMetricSamplerows in memory and writes once per set. Do the same here: buffer for the whole recording and flush once at set commit and once at cooldown end. I could not verify how the iOS native driver behaves under concurrent writes from a second thread. -
Storage and memory growth. ECG is stored as CSV text at 130 Hz, roughly 2.5 MB per recorded hour, with no cap or retention.
PolarSessionAnalyticsCard.kt:140loads the whole session's ECG, beats and everyMetricSamplerow into memory on each expanded history card, Polar or not.DataBackupManager.kt:459and:3259load every ECG segment of every session into one list and then into JSON strings, while the metric export right above streams per session on purpose to avoid OOM. Recommend opt-in raw ECG persistence, per-session streaming in the backup writer, and a retention or size cap. -
Two owners of the WorkoutSession Polar columns and a layering break.
SqlDelightWorkoutRepository.insertSessionRowwrites them at commit and the BLE manager rewrites them frompersistPolarSummarydirectly againstPhoenixDatabase.WorkoutRepository.kt:260and the composables now expose SQLDelight row types, which nothing else in that interface does. Route the writes through the repository and map rows to domain models. -
Session summary semantics.
sessionHrsatPolarH10BleManager.kt:1020keeps accumulating during the cooldown and adds the packet HR once per RR interval. Average HR and Min HR written at HRR-60 and HRR-300 are therefore pulled down by up to five minutes of recovery data and double-weighted on two-beat packets. Decide whether the summary covers the set or the set plus recovery, and compute it once. -
HistoryTab.kt:1188. A grouped routine shows the first session that has HR data as the routine's analytics. A routine has one session per set, so this hides the rest and labels one set's numbers as the routine's. -
Portal parity. Nothing in this PR reaches the portal. The push DTOs are unchanged and
persistPolarSummarydoes not mark the row dirty, so HR data is local plus backup only, and a reinstall followed by a portal pull drops it. That may be acceptable, but the PR and the release notes should say so. Since the wire format is unchanged there is nothing portal-side to verify yet. -
ActiveWorkoutScreen.kt:202andPolarH10WorkoutStrip.kt:127. Entering the workout screen starts an unfiltered 10 second BLE scan whenever a strap is remembered, and the strip starts ECG and ACC streaming on entry whether or not a set is running. Android throttles apps that start more than 5 scans in 30 seconds, and continuous ECG streaming drains the H10 quickly.PolarH10SettingsCard.kt:482also ships a "Live Telemetry Streams (Testing)" section to every user.
Minor.
DataModule.kt:1gained a UTF-8 byte order mark at byte 0.PolarH10BleManager.kt:828. Kable emits Disconnected for a fresh peripheral beforeconnect(), so the observer runscleanupSessionTelemetry()and cancels an active cooldown when the user connects a strap mid-cooldown. The trainer manager guards this withwasEverConnected.PolarH10BleManager.kt:1264treats a control-point response timeout as success, and line 1291 marks ECG as streaming even on a non-zero status, so a failed start is never retried.PolarH10BleManager.kt:1544accepts any device advertising the standard HR service, so a Garmin or Wahoo strap pairs as a "Polar H10" and then fails the PMD commands.PolarProtocolParser.kt:201decodes only the first delta block of a compressed ACC frame; Polar's reference parser loops over blocks. Posture is EMA-filtered, so this only lowers the effective rate.PolarSessionAnalyticsCard.kt:737null-checkshrBpm, a NOT NULL column.DataBackupCompletenessTest.kt:277. The comment says the Polar metric must be included when raw telemetry is off; the assertion right after expects zero.- Unused imports in the settings card and the strip:
PolarAccSample,collectLatest,AnimatedVisibility.
Ponytail.
presentation/util/PolarTelemetryUtils.kt:L1: delete, three passthroughs toutil.PolarTelemetryUtils. Import the canonical one.PolarH10WiringTest.kt:L1: delete, asserts on source text, not behaviour.MainViewModel.kt:L553: delete, the disconnect-cancels-cooldown collector duplicates the manager's own handling incleanupSessionTelemetryand in the cooldown loop.isPolarConnectedat L852 has no callers.PolarH10BleManager.kt: delete, no production callers:pmdMeasurement,hrMeasurements,rrClockTimestampMs,isEcgActive,isAccActive,currentPeripheral,cleanup().PolarSessionAnalyticsCard.kt:L115: shrink, recomputing RMSSD and average RR from beat rows and silently overriding the stored summary. Keep one source of truth.util/PolarTelemetryUtils.kt:L135: shrink,formatMs,formatRrMsandformatRmssdMsare one function.PolarH10SettingsCard.kt:L482: delete the testing toggles section before release. Product call.
Ponytail net: about -210 lines.
Impact map, minimal patch and merge guidance
Upstream and downstream effects traced beyond the findings above:
- Domain and schema.
WorkoutSessiongains 9 nullable fields,WorkoutMetric8,MetricSample8 columns, plus two tables. Everything is defaulted, everySELECT *mapper was updated, and allinsertMetricRestoreIfAbsentcallers compile. - Sync.
mergeSessionLwwis INSERT OR IGNORE andupdatePortalSessionProjectionlists its columns, so portal pulls do not null the new columns. No INSERT OR REPLACE was added, so the FK cascade rule in CLAUDE.md holds. - Deletion.
deleteSessionis a plain DELETE with foreign keys on in both drivers, so Polar rows cascade with the session. Soft-deleted sessions are excluded from backups by the new queries. - Backup. New sections default to empty, the importer sets
ignoreUnknownKeysand skips unknown sections, so older builds can still restore newer files.DeletedProfileBackupFiltergot the two parent keys. Restore order places Polar rows after sessions and checks the parent exists. - DI. One new Koin single in
dataModuleuses the plainSettingsbinding, so Polar keys land in SharedPreferences and NSUserDefaults, not the secure store. Fine. - Users without a strap. Settings always shows the Polar card. The "Start Next Exercise" button moved out of the next-exercise card. Expanding a history card now runs three extra queries including the full metric list. Nothing else changes for them: the auto-reconnect scan only runs when a device is remembered and the metric copy only when connected.
- Users with a strap. A cooldown holds the set-summary auto-continue and suppresses completion auto-navigation. Streams start when the workout screen opens.
- Release. CLAUDE.md's pending release gates require on-device BLE verification on both trainers. This adds a second BLE peripheral and a 5 minute post-workout flow to that checklist.
Minimal patch set:
- Move the Polar DDL to
59.sqmandMigrationStatements[59], revert58.sqm, retarget the parity test. - In the manager, record which session the buffers belong to, reject cooldown and summary requests for other ids, and clear
activeSessionIdand_activeCooldownSessionIdon every cooldown end. PassshowCooldown = falsefrom History. - Pass the manager down from
MainViewModelinstead ofkoinInject()in cards and screens. - Replace the new shape and colour literals with theme tokens, or raise the ratchet baselines deliberately in the same PR.
- Buffer beats and ECG for the whole recording and flush in one transaction at set commit and at cooldown end.
- Stream Polar rows per session in the backup writer.
Merge guidance: do not merge until items 1 to 4 are done and CI is green. Items 5 and 6 and the storage policy should be settled before the next release.
20552c2 to
fc40963
Compare
|
Rebased onto
|
… and unconditional backup support
…metry canvas charts
…down persistence, and chart scaling
…elow rep cards, and fix scrubber labels
…er RoutineComplete card, and filter RMSSD artifacts
…e contact is lost
…ence and RMSSD deduplication
…ty, and lifecycle changes - Schema Migration: Revert Polar DDL from migration 58 to keep custom equipment intact; create migration 59 with PolarBeatSample and PolarEcgSegment tables, updating MigrationStatements, SchemaManifest, and retargeting SchemaParityTest to expected schema version 60. - Koin DI & UI Ratchet: Decouple PolarH10BleManager injection from Compose components, threading manager down from MainViewModel through screen hierarchies; adopt MaterialTheme shapes and color tokens across Polar cards. - State Leaks & Cooldown Isolation: Guard cooldown actions and session summaries with recordedSessionId; ensure clean cancellation/restoration when routines are resumed or explicitly restarted; pass showCooldown=false for historical workouts. - Database Autocommit & Backup OOM: Buffer live telemetry samples in memory and flush in single transaction blocks upon set/cooldown completion; stream backup telemetry rows per-session rather than querying global tables. - Cleanup: Remove dead code, testing toggles, duplicate disconnect listeners, and redundant utility copies; strip UTF-8 BOM from DataModule.
…orkout HRR button
…k, and workout auto-reconnect
…ing, drop closePeripheral
fc40963 to
728f957
Compare
…tion and fix Start Cooldown on RoutineCompleteScreen
|
All findings from the review on
|
9thLevelSoftware
left a comment
9thLevelSoftware
left a comment
There was a problem hiding this comment.
Review: Polar H10 telemetry (head afbbcba)
Verdict
Request changes. CI is red, a connect timeout can leave the Polar connection stuck until the app restarts, and several paths write wrong heart-rate figures into permanent storage. The rest of the app is at low risk: schema, sync and backup are sound, and only one change (finding 6) affects users without a strap.
The Kilo "Approve / can merge as-is" verdict on this head missed everything below. It also accepted three fixes that are not in the code (finding 12).
CI. The shared suite passes (5,416/5,416) and lint passes. androidApp fails 2 tests in PersistedFileUpgradeHarnessContractTest. The cause is that the schema moved to 60, but five markers still say 59:
scripts/qa/Invoke-PhoenixPersistedFileUpgrade.ps1:L31docs/qa/persisted-file-upgrade.md:L5docs/qa/profile-schema42-fixture.md:L3,L174,L179
Because Unit Tests failed, the "Build Android" and "iOS Test Target Compile" jobs were skipped. The iOS compile of this code is therefore unverified on this head.
Correctness / Safety Findings
1. critical: PolarH10BleManager.kt:L1010-1017: a connect timeout leaves the strap stuck in Connecting until the app restarts.
withTimeoutthrowsTimeoutCancellationException.rethrowIfCancellation()rethrows it, so the CONNECT_FAIL log,cleanupExistingConnection()andConnectionState.Errorare all skipped.- Kable's following
Disconnectedis ignored by thewasEverConnectedguard.ensureConnectedOrReconnectandautoReconnectIfRememberedthen return early on every later call, and the settings card shows a spinner with no buttons. - Trigger: the strap connected earlier in the process, then the user takes it off or goes out of range, and the next set starts an auto-reconnect.
- Fix: catch
TimeoutCancellationExceptionbeforerethrowIfCancellation(), asKableBleConnectionManagerdoes at L723/L886.
2. important: PolarH10BleManager.kt:L1285, L1309, L1327, L1089-1094: a PMD subscribe failure silently kills the connection-state observer.
- The
.catch { log }blocks swallow observe errors, socpSubscribed/dataSubscribednever complete. - After 5 s,
withTimeoutthrows.onDeviceConnectedrethrows it out of thep.state.collectlambda andstateObserverJobends. - After that, disconnects are never seen. The state stays Connected, auto-reconnect never runs, and the cooldown's disconnect check never fires.
- Trigger: any non-Polar 0x180D strap (the filter accepts them, see finding 12) or a failed PMD CCCD write on a real H10.
- Fix: complete the deferreds exceptionally from
.catch, and handle the PMD timeout locally as non-fatal instead of rethrowing it.
3. critical: PolarH10BleManager.kt:L456, L1236-1245, L579: the cooldown overwrites the last set's stored summary with set-plus-recovery data.
- This is finding 10 from the 23 Oct review, and it is still present.
endSessionRecordingmakes the last set active again but never clearssessionHrs/sessionRrs, so recovery beats are appended.- At 60 s and 300 s,
persistPolarSummaryrewrites that set's average/min/max HR and RMSSD. Torso maxima also keep updating during cooldown. - This happens on every completed workout with the strap connected.
- Fix: freeze the set summary at commit, and have the cooldown write only
hrr60Bpm/hrr300Bpm.
4. critical: PolarH10BleManager.kt:L409-421: "Cancel Cooldown" (PolarSessionAnalyticsCard.kt:L504) stores mislabelled HRR.
stopCooldown()writes the drop at the moment of cancel into bothhrr60andhrr300whenever they are null, then persists them.- Cancelling at 20 s stores a 20-second drop as both HRR60 and HRR300.
- Fix: persist only values whose window actually elapsed.
5. important: PolarH10BleManager.kt:L268, ActiveSessionEngine.kt:L9574, L10863: the Polar flush runs a blocking SQLite transaction on the main thread.
persistSnapshotruns onviewModelScope, and the flush sits betweencommitCompletedSetandcommitSettled.complete, so Set Summary waits on it.stopCooldown()does the same from a button tap, with up to 5 minutes of ECG and beat rows.- The flush does not go "transactionally via WorkoutRepository" as the 29 Oct comment says. The manager still writes
PhoenixDatabasedirectly; this is finding 9 from the earlier review. - Fix: route the rows through
commitCompletedSet(or a suspend repository call onDispatchers.IO).
6. important: PolarSessionAnalyticsCard.kt:L151-169: three queries for every user, including users without a strap.
- One of them is a full
MetricSampleSELECT *, loaded only to decide whether to hide the card. - It runs on every expanded history card, every set summary and both completion screens.
ActiveSessionEngine.kt:L8099callsstartSessionRecordingunconditionally, solastRecordedSessionIdis set for everyone.- Fix: gate on the session's Polar summary columns, or use an EXISTS query, before loading rows.
7. important: PolarH10BleManager.kt:L371-389, L1150-1175, L238-246: a cancelled cooldown discards its data.
cancelCooldownneither flushes the buffered cooldown beats/ECG nor persists anything.- It fires on a single HR packet without contact (no debounce), on a 3 s packet gap, or on a disconnect.
startSessionRecordingalso clears an unflushed cooldown buffer if a new set starts within the 5 minutes.- Fix: flush whatever was recorded before clearing, and debounce contact loss.
8. important: PolarH10WorkoutStrip.kt:L130-134: leaving the screen stops the streams the cooldown needs.
- The strip calls
stopTelemetryStreamsAsync()unconditionally when it is disposed, andendSessionRecordingnever restarts the streams. - Trigger: navigating to RoutineComplete. The 5-minute cooldown then records HR only, with no ECG and no torso data.
- Fix: don't stop streams while a recording or cooldown is active.
9. important: History never shows the Polar results it stores.
PolarSessionAnalyticsCard.kt:L377gates the entire recovery section onshowCooldown, and History passesfalse(HistoryTab.kt:L583). Persisted HRR60/HRR300 are therefore never displayed.HistoryTab.kt:L1205-1207still picks the first set with HR data for a grouped routine (finding 11 from the earlier review), while the HRR is stored on the last set.- Fix: show the stored HRR chips regardless of
showCooldown, and choose the session deliberately.
10. important: WorkoutTab.kt:L292 + ActiveWorkoutScreen.kt:L309-318: possible blank screen.
CompletedCardrenders only while the trainer is connected, and auto-navigation is suppressed while a cooldown runs.- If the trainer is switched off at the end of a workout, the user sees an empty screen for up to 5 minutes. Back still works.
11. important (plausible, not verified on a device): iOS scan interference.
- Kable on Apple shares one
CentralManager, and a scanner's completion callscentral.stopScan()(mirrored inthird_party/kable-core-tvos/.../CentralManagerCoreBluetoothScanner.kt:L67). - The 10 s Polar targeted scan at set start (
PolarH10BleManager.kt:L258-261→L886) could therefore end a concurrent trainer reconnect scan. - Please cover this in the on-device release gate.
12. important: claimed fixed in earlier rounds, but unchanged since they were introduced:
PolarH10BleManager.kt:L1455-1457: a control-point response timeout is still treated as success. The response listener starts viascope.launchon areplay = 0flow, so a fast indication can also be missed.PolarH10BleManager.kt:L1889-1898: any 0x180D strap is still accepted as a Polar H10, andPolarH10BleManagerTest.kt:L52-56asserts this.PolarProtocolParser.kt:L194-224: delta-compressed ACC still decodes only the first block.- Raw ECG is still always persisted, with no opt-in and no retention cap (about 2.5 MB per recorded hour).
13. minor: ActiveSessionEngine.kt:L10780: incomplete manual-stop session. The manual-stop fallback builds its WorkoutSession without the Polar summary.
14. minor: DataBackupManager.kt:L2640, L2678: restore id collisions miscounted. A Polar row whose id already exists is counted as "skipped". The metric path compares the rows and reports a conflict instead.
15. minor: DataBackupCompletenessTest.kt:L247: misleading test. The test name and comments say data survives with raw telemetry off, but the assertions expect zero rows.
16. minor: ActiveWorkoutScreen.kt:L202 → PolarH10BleManager.kt:L842: Disconnect overridden. Every entry to the workout screen resets isExplicitDisconnect, so a Disconnect the user tapped in Settings is overridden.
17. minor: unsynchronized shared state.
discoveredAdvertisementsreads (L873/L932) race scan writes. AConcurrentModificationExceptionthere has no handler on the scope and would crash the app.endHeartRate!!(L523) can race to null.- Both windows are narrow.
18. minor: PolarSessionAnalyticsCard.kt:L172-176: global strap state on unrelated cards. While a strap is connected or a cooldown runs, unrelated history cards show an empty "Connected" card or "Recovery Cooldown Active".
19. minor: PolarSessionAnalyticsCard.kt:L325, L530: clinical-sounding labels. "High Strain" and "Elevated Risk" are derived from non-standardised post-set data.
Cascading checks that came out clean:
- Schema:
59.sqm,PhoenixDatabase.sq,SchemaManifestandMigrationStatements[59]match, and the change is additive. NoINSERT OR REPLACEwas added (insertSessionis a plain INSERT and the summary uses UPDATE). Rows cascade on session delete. - Portal pulls: they do not null the new columns.
- Backups: older builds ignore the new sections, and the deleted-profile filter covers both tables.
- tvOS: it receives no Polar manager.
- Portal parity: no HR data reaches the portal, so a reinstall followed by a pull loses all of it. The PR description and release notes should say so.
Ponytail Review
MainViewModel.kt:L839: deleteisPolarConnected, which has no callers. Nothing replaces it.SqlDelightWorkoutRepository.kt:L25-26: delete the unusedPolarTelemetryUtilsandkotlin.math.absimports.PolarSessionAnalyticsCard.kt:L127-146: shrink the code that recomputes RMSSD and average RR from beat rows and silently overrides the stored summary. Keep one source of truth.DefaultWorkoutSessionManager.kt:L537,L565: delete the dead?: coordinator.currentSessionIdfallback and callendSessionRecording()(Kilo's open suggestion).Models.kt,BackupModels.kt: delete the stale "migration 58" comments. These columns ship in 59.
Ponytail net: -40 lines.
Suggested Minimal Patch
- Bump the five schema markers to 60. This makes CI green.
PolarH10BleManager.connect: catchTimeoutCancellationExceptionand reach the error/cleanup path.ensurePmdSubscribed: fail the deferreds from.catch, and treat a PMD timeout as non-fatal insideonDeviceConnected.- Cooldown: stop appending to
sessionHrs/sessionRrs/torso maxima; write HRR columns only; persist only HRR windows that elapsed; flush before every cancel. - Move the flush off the main thread, ideally into
commitCompletedSet. - Gate the card's queries on the session's Polar columns.
Final Merge Guidance
Do not merge until CI is green and findings 1, 3 and 4 are fixed. Findings 2 and 5-10 should land in the same PR. Finding 12 and the ECG storage policy must be settled before the next release.
Generated by Claude Code
|
Implemented the latest review findings and remaining earlier feedback in the pushed revision. The complete mapping is in The fixes cover timeout/PMD lifecycle recovery; frozen set summaries and elapsed-only HRR; fresh-HR and contact-loss handling; queued IO repository persistence with source-ID ownership across delayed commits; non-blocking deleted-parent handling; history/query/completion behavior; complete ACC delta parsing; raw ECG consent and retention; backup conflicts; and schema-60 QA markers. Removed unused/redundant paths and shipped testing controls. Polar data's local/file-backup-only limitation is explicit in the PR description and release notes. Validation on Windows:
Hosted unit tests passed on The next hosted unit run exposed a preexisting scheduler race in the deferred PR-weight conversion test (5,440 of 5,441 passed). With explicit owner authorization, iOS main compilation then cleared the dispatcher issue; iOS test compilation identified commas in one parser test name. Final hosted results on
The iOS concurrent-scan concern is recorded as a physical-device release gate rather than claimed fixed without hardware evidence. Physical BLE/native iOS verification remains separate from local and hosted results. At final verification GitHub reports the review decision as approved. No merge or deployment was performed as part of this remediation. |
| command = BleConstants.Polar.PMD_STOP_ECG_CMD, | ||
| opCode = 0x03, | ||
| measurementType = 0x00, | ||
| ).getOrThrow().also { check(it == 0) { "Polar H10 rejected ECG stop: status $it" } } |
There was a problem hiding this comment.
WARNING: A failed STOP now leaves ECG flagged as streaming for the rest of the session.
sendPmdControlCommand throws on a control-point timeout (:1546) and this new .getOrThrow().also { check(it == 0) } throws on a non-zero status, so everything after it is skipped: _isEcgStreaming.value = false (:1656), _latestEcg.value = null (:1657) and checkAndTeardownPmdObservers() (:1658). The manager then still reports ECG streaming with live PMD observers, so ensureRecordingTelemetryStreams() (:779-781) never re-issues START_ECG and the watchdog (:1828-1835) sees a healthy stream and never repairs it - ECG capture is dead for every later set. stopAccStream() (:1758) has the same shape.
Required fix: reset the streaming flags, clear the cached samples and call checkAndTeardownPmdObservers() in a finally (or before the throw), so a failed STOP leaves a state the watchdog can restart from.
| error.rethrowIfCancellation() | ||
| _telemetryPersistenceError.value = error.message ?: "Polar telemetry could not be saved" | ||
| log.e(error) { "Failed to flush Polar telemetry for ${pending.sessionId}" } | ||
| break |
There was a problem hiding this comment.
WARNING: One failed batch permanently blocks every later telemetry flush.
The catch keeps the failed batch at the head of pendingTelemetry (:420 removes it only on success) and breaks out of the drain loop. The next queueTelemetry call re-reads pendingTelemetry.firstOrNull() (:390) - the same batch - and breaks again, so no later session's beats or ECG are ever written, while the queue grows by one entry per flush. _telemetryPersistenceError (:424) is the only signal and no screen consumes it.
Required fix: move a failed batch to the back of the queue (or drop it after N attempts) and bound pendingTelemetry, so one poisoned batch cannot starve every later recording.
| val samples = mutableListOf<PolarAccSample>() | ||
| var offset = 10 | ||
| while (offset + axisByteCount * 3 <= data.size) { | ||
| val x = if (axisByteCount == 1) data[offset].toInt() else getInt16LE(data, offset) |
There was a problem hiding this comment.
WARNING: 8-bit ACC counts are fed into a milli-g pipeline.
PolarAccSample is milli-G (PolarModels.kt:31-37), and everything downstream assumes it: accFilter.processSample(s.xMg, s.yMg, s.zMg) (PolarH10BleManager.kt:1485) -> PolarTelemetryUtils.calculateDynamicAccMg computes abs(|a| - 1000.0) with 1000 mg = 1 g. The new axisByteCount == 1 branch returns raw counts (the new test asserts PolarAccSample(127, -128, -1)), so a motionless wearer yields a dynamic acceleration of roughly 1000 mg instead of roughly 0, and that value is stored per beat (PolarBeatSample.dynamicAccMg) and charted as dynamic acceleration. Pitch and roll survive because atan2 ratios are scale-invariant.
Required fix: convert the 8-bit counts to milli-G with a documented scale factor (or reject 8-bit frames with return null) until a device capture establishes the unit.
|
|
||
| updateSessionPolarRecovery: | ||
| UPDATE WorkoutSession SET | ||
| hrr60Bpm = :hrr60Bpm, |
There was a problem hiding this comment.
WARNING: This recovery update clears a stored HRR-300 when only HRR-60 is known.
Both columns are assigned unconditionally, and the repository always calls this query with the current in-memory pair (SqlDelightPolarTelemetryRepository.kt:57). At the 60 s mark of a cooldown only hrr60 is set, so hrr300Bpm is written as NULL - normally harmless, because that column is NULL anyway. It stops being harmless after the Restart Cooldown button (PolarSessionAnalyticsCard.kt:539-544): that path resets the in-memory hrr60/hrr300 (PolarH10BleManager.kt:559-560) while the row still holds the old values, so the next 60 s write erases the stored HRR-300. Cancel the restarted cooldown and that figure is gone for good.
Required fix: make the recovery write non-destructive - hrr60Bpm = COALESCE(:hrr60Bpm, hrr60Bpm), hrr300Bpm = COALESCE(:hrr300Bpm, hrr300Bpm) - so an absent measurement never clears a stored one.
| repMetrics = snapshot.repMetrics, | ||
| repBiomechanics = snapshot.biomechanicsRepResults, | ||
| ) | ||
| polarBleManager?.flushSessionTelemetry(sessionId)?.join() |
There was a problem hiding this comment.
SUGGESTION: This join() sits between the durable commit and the signal that the set is durable.
commitCompletedSet (:9567) has already written the set, but join() is a cancellation point. If the scope is cancelled in that window the CancellationException is caught at :9614, which calls executionGuard.markPersistenceFailed(sessionId) and rethrows - so a set that is already saved is reported as failed, retained for replay, and commitSettled (:9576) is never completed.
Required fix: complete commitSettled before joining the telemetry flush, or run the join under withContext(NonCancellable) so a cancelled scope cannot retract a committed set.
| settings?.putBoolean(KEY_RAW_ECG_STORAGE, enabled) | ||
| sessionLock.withLock { | ||
| _isRawEcgStorageEnabled.value = enabled | ||
| if (!enabled) bufferedEcgSegments.clear() |
There was a problem hiding this comment.
SUGGESTION: Switching raw ECG storage off leaves the sample budget and the 5-minute window in place.
rawEcgStartMs / rawEcgSampleCount are only reset in startSessionRecording (:321-322), so after the buffer is cleared the counters still describe the old capture. Re-enabling the toggle mid-set resumes with the exhausted 39 000-sample budget and the original start point, and monotonicClock() - startMs >= MAX_RAW_ECG_DURATION_MS (:1456) then silently stops storing ECG for the rest of the session.
Required fix: reset rawEcgStartMs and rawEcgSampleCount together with the buffer, or do not clear the buffer at all.
| Spacer(modifier = Modifier.height(Spacing.medium)) | ||
| Text( | ||
| text = if (polarSession.hrr60Bpm != null || polarSession.hrr300Bpm != null) { | ||
| "Latest set with recovery data: ${polarSession.exerciseName}" |
There was a problem hiding this comment.
SUGGESTION: exerciseName is nullable, so this label renders null.
WorkoutSession.exerciseName is String? (Models.kt:588) and is null for free-form and legacy rows. Every other label in this file falls back (HistoryTab.kt:308 uses session.exerciseName ?: ...).
Required fix: use polarSession.exerciseName ?: + "Unknown Exercise" + here and at:1223`.
| // through connect() without self-cancelling reconnectJob | ||
| manager.startSessionRecording("session-123", "WORKING_SET") | ||
|
|
||
| assertTrue(manager.isExplicitDisconnectForTesting) |
There was a problem hiding this comment.
SUGGESTION: The comment above still describes the opposite contract.
Lines 868-869 say 'Starting a workout should clear the explicit disconnect suppression and complete auto-reconnect through connect()', but these assertions were flipped to expect the opposite (:872-873): an explicit disconnect now survives startSessionRecording and no reconnect is attempted. The test name was updated; the comment was not.
Required fix: rewrite the comment to state the new contract (an explicit Disconnect survives the start of a recording, and screen entry no longer re-initiates a reconnect).
Summary
Adds optional Polar H10 telemetry alongside trainer BLE: heart rate and R-R intervals, battery, ECG preview, accelerometer-derived torso movement, session summaries and five-minute post-workout recovery.
Behavior
Portal limitation: Polar summaries and raw telemetry are local plus exported file backup only. They are not synchronized to Phoenix Portal. Reinstalling and pulling from the portal does not restore Polar data.
Review resolution and validation
The October 29 findings and remaining earlier feedback are mapped in the resolution report.
-Pskip.supabase.check=true; they do not establish live portal/provider behavior.f6860676. Native compilation identified a missing multiplatformkotlinx.coroutines.IOimport, fixed in2b112607; its Kotlin native TV unit-test stage subsequently passed.9dd0dc8agives that single test control of Main, drains initialization before mutation, and replaces wall-clock polling with a direct assertion. The exact previously failing case passed locally, then hosted Unit Tests and Lint Check passed.a29fb717renames that test without changing its assertions.a29fb717passed CI Tests: Unit Tests, lint, Android debug build, iOS main/test-target compilation, schema and security checks. The optional iOS simulator-test job was skipped as configured.Release gates
Physical-device checks remain required on Android and iOS with both trainer models, including iOS concurrent trainer/Polar scanning and native SQLite behavior. Hosted compilation and tests do not substitute for these checks. At final verification GitHub reports the review decision as approved. No merge or deployment was performed as part of this remediation.