From 3f5e0b8c25f8e9f4ea33a22b08fd623e584ea2f6 Mon Sep 17 00:00:00 2001 From: leona Date: Mon, 5 Oct 2026 15:40:50 +0200 Subject: [PATCH 01/33] Add phone-based step source (Health Connect) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds an optional alternative source for daily step data: the phone's own pedometer, read from Health Connect. Users select the source in Settings → Step Source; the default Ring mode is unchanged. - PhoneStepManager reads today's total and backfills up to 30 days of history, anchored to local midnight so Health Connect's per-day buckets land on the correct calendar dates. - PhoneStepsUpdate carries only the step count. Distance is derived on write from the profile's height (stride); calories are left at 0 so DailyCalorieEstimator's BMR + net-active figure surfaces. - EventPersistenceSubscriber routes the update into activity_daily with source = "phone". Both ring paths (live ActivityUpdate and bucket-derived daily total) skip the row while the phone owns it. - SettingsScreen requests READ_STEPS only when the user switches the toggle to Phone. MainActivity never requests it at launch or on resume, so Ring users never see the prompt. - StepSourcePrefs stores the preference in plain SharedPreferences; ApiKeyStore no longer carries stepSource. --- app/src/main/AndroidManifest.xml | 10 +- .../main/java/com/pulseloop/MainActivity.kt | 56 ++++ .../java/com/pulseloop/PhoneStepManager.kt | 239 ++++++++++++++++++ .../health/HealthConnectTypeMappings.kt | 19 +- .../java/com/pulseloop/ring/PulseEventBus.kt | 27 +- .../service/DailyCalorieEstimator.kt | 23 +- .../service/EventPersistenceSubscriber.kt | 145 +++++++++-- .../com/pulseloop/settings/ApiKeyStore.kt | 5 +- .../com/pulseloop/settings/StepSourcePrefs.kt | 48 ++++ .../pulseloop/ui/screens/ActivityScreen.kt | 37 ++- .../pulseloop/ui/screens/SettingsScreen.kt | 119 ++++++++- .../com/pulseloop/ui/screens/TodayScreen.kt | 15 +- .../com/pulseloop/ui/viewmodels/ViewModels.kt | 27 +- 13 files changed, 720 insertions(+), 50 deletions(-) create mode 100644 app/src/main/java/com/pulseloop/PhoneStepManager.kt create mode 100644 app/src/main/java/com/pulseloop/settings/StepSourcePrefs.kt diff --git a/app/src/main/AndroidManifest.xml b/app/src/main/AndroidManifest.xml index 61b16739..2898a355 100644 --- a/app/src/main/AndroidManifest.xml +++ b/app/src/main/AndroidManifest.xml @@ -42,7 +42,9 @@ + first permission sheet stayed narrow. READ_STEPS below is the one exception: it supports + the optional phone step source, and users on the default "Ring" source never see the + Health Connect read prompt and never trigger a read. --> @@ -62,6 +64,10 @@ + + - + \ No newline at end of file diff --git a/app/src/main/java/com/pulseloop/MainActivity.kt b/app/src/main/java/com/pulseloop/MainActivity.kt index a6dbaa15..4c994467 100644 --- a/app/src/main/java/com/pulseloop/MainActivity.kt +++ b/app/src/main/java/com/pulseloop/MainActivity.kt @@ -12,6 +12,7 @@ import androidx.activity.result.contract.ActivityResultContracts import androidx.core.content.ContextCompat import com.pulseloop.health.HealthConnectPermissionReconcile import com.pulseloop.notifications.CoachNotifications +import com.pulseloop.settings.StepSourcePrefs import com.pulseloop.strava.StravaAuth import com.pulseloop.strava.StravaTokenStore import com.pulseloop.ui.PulseLoopApp @@ -25,6 +26,13 @@ import kotlinx.coroutines.launch * - Android 12+: BLUETOOTH_SCAN + BLUETOOTH_CONNECT * - Android < 12: ACCESS_FINE_LOCATION (for BLE scanning) * - Android 13+: POST_NOTIFICATIONS + * + * Health Connect READ_STEPS is deliberately **not** requested here — it is + * requested only from the Step Source row in Settings, on the user's explicit + * action. This means users on the default "Ring" step source never see the + * prompt, and users who declined are not asked again on every foreground. + * ACTIVITY_RECOGNITION is not requested at all: the fork reads steps from + * Health Connect, never from the raw hardware step counter. */ class MainActivity : ComponentActivity() { @@ -68,6 +76,7 @@ class MainActivity : ComponentActivity() { super.onResume() // Re-request on resume in case user granted in Settings requestAllPermissions() + refreshPhoneSteps() if (hasAllBlePermissions() && hasNotificationPermission()) { CoachNotifications.schedule(this) } @@ -193,4 +202,51 @@ class MainActivity : ComponentActivity() { private fun hasFineLocation() = ContextCompat.checkSelfPermission( this, Manifest.permission.ACCESS_FINE_LOCATION ) == PackageManager.PERMISSION_GRANTED + + /** + * Refresh today's phone step count and backfill the last 30 days from + * Health Connect, when the user's step source is set to "Phone". + * + * If the Health Connect read permission is missing, this quietly returns + * without prompting. The permission is requested from Settings → Step + * Source, on the user's explicit action — never from here, so neither app + * launch nor return-to-foreground re-triggers a dialog the user may have + * already declined. + * + * On failure, publishes [PulseEvent.PhoneStepSourceUnavailable] so the + * persistence layer can fall back to the ring (A5) — a phone mode that + * cannot actually read steps must not silently gate the ring's own + * totals off forever. + */ + private fun refreshPhoneSteps() { + val prefs = StepSourcePrefs(this) + if (prefs.stepSource != StepSourcePrefs.SOURCE_PHONE) return + + lifecycleScope.launch(Dispatchers.IO) { + val manager = PhoneStepManager(this@MainActivity) + if (!manager.refresh()) { + // A5: the phone reader couldn't produce a value — permission revoked, the + // provider uninstalled, or a device with no Health Connect. Signal the + // persistence layer so it stops gating the ring's live totals off; without + // this, the preference staying on "phone" would freeze today's count. + com.pulseloop.ring.PulseEventBus.publishBlocking( + com.pulseloop.ring.PulseEvent.PhoneStepSourceUnavailable + ) + return@launch + } + // A10: the 30-day historical backfill runs at most once per local day. It has to + // run today's refresh on every foreground — the phone has been walking since the + // last one — but re-reading 30 days of Health Connect on every resume, writing + // every row's updatedAt forward, and having the exporter re-select and re-export + // the whole tail is wasted work. The toggle in SettingsScreen still calls + // refreshHistoricalDays() directly and is not throttled: an explicit user action + // should always run. + val today = com.pulseloop.util.TimeUtil.startOfTodayLocal() + if (prefs.lastBackfillDay != today) { + if (manager.refreshHistoricalDays()) { + prefs.lastBackfillDay = today + } + } + } + } } diff --git a/app/src/main/java/com/pulseloop/PhoneStepManager.kt b/app/src/main/java/com/pulseloop/PhoneStepManager.kt new file mode 100644 index 00000000..75ff51b0 --- /dev/null +++ b/app/src/main/java/com/pulseloop/PhoneStepManager.kt @@ -0,0 +1,239 @@ +package com.pulseloop + +import android.content.Context +import androidx.health.connect.client.HealthConnectClient +import androidx.health.connect.client.permission.HealthPermission +import androidx.health.connect.client.records.StepsRecord +import androidx.health.connect.client.records.metadata.DataOrigin +import androidx.health.connect.client.request.AggregateGroupByPeriodRequest +import androidx.health.connect.client.request.AggregateRequest +import androidx.health.connect.client.time.TimeRangeFilter +import com.pulseloop.ring.PulseEvent +import com.pulseloop.ring.PulseEventBus +import com.pulseloop.util.TimeUtil +import java.time.Instant +import java.time.LocalDate +import java.time.LocalDateTime +import java.time.Period +import java.time.ZoneId + +/** + * Reads today's step total from Health Connect and publishes it on the bus. + * + * Why Health Connect instead of the raw TYPE_STEP_COUNTER hardware sensor? + * - The hardware sensor reports cumulative steps since the phone's last reboot, not + * steps today. There is no way to recover "how many steps before midnight" from it. + * - Health Connect aggregates step records per-day, handles reboots, and always has + * the correct "today" total — even for hours when PulseLoop was closed. + * - Health Connect is fully on-device (Android system service), so reading from it + * does not compromise the app's privacy-first design. No data leaves the phone. + * + * This class has no long-lived listeners or timers — it queries on demand, when the + * app comes to the foreground. Zero battery cost while the app is closed. + * + * Only `steps` is published. Distance and calories are not the phone reader's job: + * calories come from [com.pulseloop.service.DailyCalorieEstimator], and workout + * distance from [com.pulseloop.service.ActivityRollup]. Baking either into the event + * would overwrite those paths on write. + * + * **Every read subtracts PulseLoop's own origin.** PulseLoop exports its own daily step + * totals to Health Connect (`ActivityExporter`), so a naive aggregate would include our + * own earlier exports — the ring's daily total fed back to the phone path, or the + * phone's own total plus the ring's. Both reads therefore run twice: once with no origin + * filter (all origins), once filtered to PulseLoop's own package, and the second is + * subtracted from the first. `ActivityExporter` also skips `source == "phone"` rows, + * which stops the loop from growing; this read-side subtraction cleans up the exports + * that already exist. + */ +class PhoneStepManager(private val context: Context) { + + /** + * PulseLoop's own origin, as Health Connect identifies it. Passed as the + * `dataOriginFilter` for the "own contribution" aggregate that gets subtracted from + * the unfiltered one — see the class KDoc. + */ + private val ownOrigin: Set = setOf(DataOrigin(context.packageName)) + + /** + * Query Health Connect for today's step total and publish it as a + * [PulseEvent.PhoneStepsUpdate]. Returns true when the query succeeded, false when + * Health Connect is unavailable or the read permission has not been granted yet. + */ + suspend fun refresh(): Boolean { + val client = try { + HealthConnectClient.getOrCreate(context) + } catch (_: Exception) { + return false + } + + val readStepsPermission = HealthPermission.getReadPermission(StepsRecord::class) + val granted = try { + client.permissionController.getGrantedPermissions() + } catch (_: Exception) { + return false + } + if (readStepsPermission !in granted) return false + + val now = Instant.now() + val startOfDayMs = TimeUtil.startOfDayLocal(System.currentTimeMillis()) + val startOfDay = Instant.ofEpochMilli(startOfDayMs) + + // Two aggregates and a subtraction: all origins minus PulseLoop's own contribution. + // See the class KDoc for why. The result is floored at 0 because a momentarily + // inconsistent pairing of the two calls could otherwise produce a negative step + // count — not a value we would ever want to publish. + val allSteps = aggregateToday(client, startOfDay, now, dataOriginFilter = emptySet()) + ?: return false + val ownSteps = aggregateToday(client, startOfDay, now, dataOriginFilter = ownOrigin) + ?: return false + val steps = (allSteps - ownSteps).coerceAtLeast(0) + + // Just after midnight, or on a phone with no step writer in Health Connect, the + // aggregate returns null and becomes 0. Publishing 0 would clobber the ring's + // only valid record for the day and mark the row source = "phone", which then + // also blocks the ring's bucket-derived total for today. Skip the publish and + // let the ring's number stand until the phone has actually counted some steps. + // The query itself succeeded, so still return true. + if (steps <= 0) return true + PulseEventBus.publishBlocking( + PulseEvent.PhoneStepsUpdate( + timestamp = now, + steps = steps, + ) + ) + return true + } + + /** + * One aggregate over today's `StepsRecord`s, filtered to [dataOriginFilter] when + * non-empty. Returns null on any client error so the caller can short-circuit. + */ + private suspend fun aggregateToday( + client: HealthConnectClient, + startOfDay: Instant, + now: Instant, + dataOriginFilter: Set, + ): Int? = try { + client.aggregate( + AggregateRequest( + metrics = setOf(StepsRecord.COUNT_TOTAL), + timeRangeFilter = TimeRangeFilter.between(startOfDay, now), + dataOriginFilter = dataOriginFilter, + ) + )[StepsRecord.COUNT_TOTAL]?.toInt() ?: 0 + } catch (_: Exception) { + null + } + + /** + * Query Health Connect for step totals per day across a rolling window, and publish each + * one as a separate [PulseEvent.PhoneStepsUpdate]. Used on app start / toggle change to + * backfill historical days from the phone's own step counter, so past days in Phone mode + * show the phone's numbers rather than whatever the ring recorded. + * + * Days where Health Connect has no phone-step records are skipped — those days fall back + * to the ring's data, which is the only source available for them. A day where the phone + * walked zero steps would publish `0` and clobber the ring's only valid record for that + * day, so the `steps <= 0` guard skips those buckets entirely. + * + * **Three Health Connect quirks handled here:** + * + * 1. `aggregateGroupByPeriod` requires the `TimeRangeFilter` to be built with + * `LocalDateTime`, not `Instant`. Passing an Instant-based filter throws + * `IllegalArgumentException: Either use TimeRangeFilter with LocalDateTime or + * AggregateGroupByDurationRequest`. The plain `aggregate` (today's query, above) + * accepts Instant, but this one does not. + * + * 2. `aggregateGroupByPeriod` aligns buckets to the range **start**, not to calendar + * days. If the range starts at `LocalDateTime.now()` (e.g. 22:42), the last bucket + * runs 22:42 yesterday → 22:42 today and gets mislabeled as "yesterday", duplicating + * today's data. We anchor the range to local midnight so every bucket is exactly one + * calendar day. + * + * 3. Every emitted total is the **difference** between the unfiltered aggregate and the + * PulseLoop-only aggregate — see the class KDoc. The two calls share the range and + * slicer, so their buckets line up by `startTime`; a bucket present only in the + * unfiltered result is a day where PulseLoop has no records, so the subtraction is + * just the unfiltered value. + * + * @param daysBack how many days of history to backfill (default 30). + * @return true if the query succeeded (even if some days were empty), false on failure. + */ + suspend fun refreshHistoricalDays(daysBack: Long = 30): Boolean { + val client = try { + HealthConnectClient.getOrCreate(context) + } catch (_: Exception) { + return false + } + + val readStepsPermission = HealthPermission.getReadPermission(StepsRecord::class) + val granted = try { + client.permissionController.getGrantedPermissions() + } catch (_: Exception) { + return false + } + if (readStepsPermission !in granted) return false + + // Anchor the range to local midnight so each bucket is exactly one calendar day. + // endLocal is tomorrow-midnight (exclusive upper bound); startLocal is the first + // day of the window. With daysBack = 30 this yields 30 calendar-day buckets, the + // last of which is today. + val todayStart: LocalDateTime = LocalDate.now().atStartOfDay() + val endLocal: LocalDateTime = todayStart.plusDays(1) + val startLocal: LocalDateTime = todayStart.minusDays(daysBack - 1) + + // Two aggregateGroupByPeriod calls, same range and slicer, differing only in the + // origin filter. See the class KDoc. + val allBuckets = try { + client.aggregateGroupByPeriod( + AggregateGroupByPeriodRequest( + metrics = setOf(StepsRecord.COUNT_TOTAL), + timeRangeFilter = TimeRangeFilter.between(startLocal, endLocal), + timeRangeSlicer = Period.ofDays(1), + dataOriginFilter = emptySet(), + ) + ) + } catch (_: Exception) { + return false + } + val ownBuckets = try { + client.aggregateGroupByPeriod( + AggregateGroupByPeriodRequest( + metrics = setOf(StepsRecord.COUNT_TOTAL), + timeRangeFilter = TimeRangeFilter.between(startLocal, endLocal), + timeRangeSlicer = Period.ofDays(1), + dataOriginFilter = ownOrigin, + ) + ) + } catch (_: Exception) { + return false + } + // Index PulseLoop's own per-day totals by bucket start time so the subtraction below + // is a single map lookup per bucket. A bucket only in `allBuckets` is a day where + // PulseLoop has no records — the subtraction is just `all - 0`. + val ownByStart: Map = ownBuckets.associate { bucket -> + bucket.startTime to (bucket.result[StepsRecord.COUNT_TOTAL]?.toInt() ?: 0) + } + + // Each bucket is one day's total. The bucket's startTime marks the beginning of the + // period it covers (local midnight). Map each non-empty bucket to a PhoneStepsUpdate + // so the persistence layer overwrites the corresponding activity_daily row. + for (bucket in allBuckets) { + val all = bucket.result[StepsRecord.COUNT_TOTAL]?.toInt() ?: 0 + val own = ownByStart[bucket.startTime] ?: 0 + val steps = (all - own).coerceAtLeast(0) + // Skip empty days: publishing 0 would clobber the ring's only valid record for + // that day. Leaving the day untouched lets it fall back to the ring's data. + if (steps <= 0) continue + PulseEventBus.publishBlocking( + PulseEvent.PhoneStepsUpdate( + timestamp = bucket.startTime + .atZone(ZoneId.systemDefault()) + .toInstant(), + steps = steps, + ) + ) + } + return true + } +} \ No newline at end of file diff --git a/app/src/main/java/com/pulseloop/health/HealthConnectTypeMappings.kt b/app/src/main/java/com/pulseloop/health/HealthConnectTypeMappings.kt index a1a77bf3..5fb321d2 100644 --- a/app/src/main/java/com/pulseloop/health/HealthConnectTypeMappings.kt +++ b/app/src/main/java/com/pulseloop/health/HealthConnectTypeMappings.kt @@ -4,6 +4,7 @@ import androidx.health.connect.client.records.ExerciseSessionRecord import androidx.health.connect.client.records.MealType import androidx.health.connect.client.records.SleepSessionRecord import com.pulseloop.ring.SleepStage +import com.pulseloop.settings.StepSourcePrefs import java.time.Instant import java.time.ZoneId import java.time.ZoneOffset @@ -24,9 +25,19 @@ import java.time.ZoneOffset */ object HealthConnectTypeMappings { - /** [com.pulseloop.data.entity.MeasurementEntity.sourceRaw] values that must never reach - * Health Connect (mirrors iOS, which never exports demo/mock data). */ - val EXCLUDED_SOURCES = setOf("demo", "mock") + /** + * `source` / `sourceRaw` values whose rows must never be exported to Health Connect. + * + * - `demo`, `mock`: seeded placeholder data (mirrors iOS, which never exports demo/mock). + * - `phone`: a phone-sourced daily total is *Health Connect's own data* — the phone reader + * got it from Health Connect in the first place. Re-exporting it would create a + * self-feedback loop: PulseLoop writes the phone's step count back, then reads it back + * as if it were the phone's, then writes it again. [com.pulseloop.PhoneStepManager] also + * filters PulseLoop's own origin out at read time; this exclusion and that filter are + * the two halves of the same fix. See `ActivityExporter.build` for the filtering call + * site. + */ + val EXCLUDED_SOURCES = setOf("demo", "mock", StepSourcePrefs.SOURCE_PHONE) const val HOUR_MS = 3_600_000L @@ -734,4 +745,4 @@ object HealthConnectTypeMappings { "snack" -> MealType.MEAL_TYPE_SNACK else -> MealType.MEAL_TYPE_UNKNOWN } -} +} \ No newline at end of file diff --git a/app/src/main/java/com/pulseloop/ring/PulseEventBus.kt b/app/src/main/java/com/pulseloop/ring/PulseEventBus.kt index 4c67857e..a00a205c 100644 --- a/app/src/main/java/com/pulseloop/ring/PulseEventBus.kt +++ b/app/src/main/java/com/pulseloop/ring/PulseEventBus.kt @@ -64,15 +64,38 @@ sealed class PulseEvent { if (this === other) return true if (other !is RawPacket) return false return direction == other.direction && data.contentEquals(other.data) && - decoded == other.decoded && deviceType == other.deviceType + decoded == other.decoded && deviceType == other.deviceType } override fun hashCode(): Int = 31 * (31 * (31 * direction.hashCode() + data.contentHashCode()) + decoded.hashCode()) + - (deviceType?.hashCode() ?: 0) + (deviceType?.hashCode() ?: 0) } data class ActivityUpdate(val timestamp: java.time.Instant, val steps: Int, val distanceMeters: Double, val calories: Double) : PulseEvent() data class ActivityBucket(val timestamp: java.time.Instant, val steps: Int, val distanceMeters: Double) : PulseEvent() data object ActivitySyncReset : PulseEvent() + /** Steps counted by the phone's own pedometer, published by the app's + * [com.pulseloop.PhoneStepManager] when the user has chosen "phone" as their step source. + * Distinct from [ActivityUpdate], which carries the ring's cumulative counter. + * + * Only `steps` is carried. Distance and calories are deliberately not baked in here: + * calories come from [com.pulseloop.service.DailyCalorieEstimator] (which needs the + * user's weight, HR, and workout history), and workout distance is credited to the + * day by [com.pulseloop.service.ActivityRollup]. Writing either from this event would + * overwrite what those paths computed. */ + data class PhoneStepsUpdate( + val timestamp: java.time.Instant, + val steps: Int, + ) : PulseEvent() + + /** + * The phone step reader failed to produce a value this pass — Health Connect unreachable, + * or READ_STEPS not granted. Published by [com.pulseloop.MainActivity.refreshPhoneSteps] + * on a `refresh()` false return, so [com.pulseloop.service.EventPersistenceSubscriber] can + * stop gating the ring's live totals off (see its `stepSourceIsPhone`) and let the ring + * fill in until the phone reader recovers. Without this, a revoked permission would drop + * the ring's contributions indefinitely while the preference still read "phone". + */ + data object PhoneStepSourceUnavailable : PulseEvent() /** * [spot] marks the one settled reading a spot measurement publishes for itself (issue #60); * false for the ring's live stream. [ringWillLogIt] additionally says this ring writes that diff --git a/app/src/main/java/com/pulseloop/service/DailyCalorieEstimator.kt b/app/src/main/java/com/pulseloop/service/DailyCalorieEstimator.kt index f36776d3..2fcf9260 100644 --- a/app/src/main/java/com/pulseloop/service/DailyCalorieEstimator.kt +++ b/app/src/main/java/com/pulseloop/service/DailyCalorieEstimator.kt @@ -38,6 +38,14 @@ object DailyCalorieEstimator { */ const val RING_HISTORY_SOURCE = "ring_history" + /** + * The `source` value [com.pulseloop.service.EventPersistenceSubscriber] writes for a + * phone-sourced day. Like `ring_history`, the row's `calories` column is not a + * device-reported figure — the phone reader supplies steps only — so it must not be treated + * as one by [deviceReportedCalories]. + */ + const val PHONE_SOURCE = com.pulseloop.settings.StepSourcePrefs.SOURCE_PHONE + data class Profile(val sex: String?, val age: Int?, val weightKg: Double?, val heightCm: Double?) /** Mifflin-St Jeor BMR (kcal/day), clamped at 0 like iOS's `mifflinBMR`. */ @@ -83,9 +91,16 @@ object DailyCalorieEstimator { * This was inverted on first port (it returned the value *only* for `ring_history`, and ignored * genuine device calories from every other source), which made the estimate replace real data * and real data replace the estimate. + * + * A `phone`-sourced row is excluded for the same reason as `ring_history`: the phone reader + * supplies steps only, so its `calories` column is never a device-reported figure — it's + * either the estimator's own fill-in or the hardcoded `steps * 0.04` placeholder this fork + * wrote before that path existed. Excluding it lets [effectiveCalories] fall through to + * `estimatedActiveCalories` on the same row. */ fun deviceReportedCalories(day: ActivityDailyEntity): Double? = - if (day.source == RING_HISTORY_SOURCE || day.calories <= 0.0) null else day.calories + if (day.source == RING_HISTORY_SOURCE || day.source == PHONE_SOURCE || day.calories <= 0.0) null + else day.calories /** * What to display: the device's own figure when it reported one, else the estimated **total** — @@ -137,7 +152,7 @@ object DailyCalorieEstimator { val buckets = db.activityBucketDao().byDay(dayStart) val workouts = db.activitySessionDao().recent(WORKOUT_SCAN_LIMIT).filter { it.statusRaw == "finished" && it.endedAt != null && - it.startedAt < dayEnd && it.endedAt!! > dayStart + it.startedAt < dayEnd && it.endedAt!! > dayStart } val active = estimateNetActive( @@ -227,7 +242,7 @@ object DailyCalorieEstimator { if (bucket.steps <= 0) continue val bucketEnd = bucket.startEpoch + (BUCKET_DURATION_SECONDS * 1000).toLong() val keepFraction = 1.0 - - overlapSeconds(Window(bucket.startEpoch, bucketEnd), covered) / BUCKET_DURATION_SECONDS + overlapSeconds(Window(bucket.startEpoch, bucketEnd), covered) / BUCKET_DURATION_SECONDS if (keepFraction <= 0.0) continue val durationMinutes = BUCKET_DURATION_SECONDS / 60.0 val cadence = bucket.steps / durationMinutes @@ -306,4 +321,4 @@ object DailyCalorieEstimator { * Android pages, and a day can only overlap sessions from that day and its neighbours. */ private const val WORKOUT_SCAN_LIMIT = 50 -} +} \ No newline at end of file diff --git a/app/src/main/java/com/pulseloop/service/EventPersistenceSubscriber.kt b/app/src/main/java/com/pulseloop/service/EventPersistenceSubscriber.kt index 8cd61fc3..92fe4d97 100644 --- a/app/src/main/java/com/pulseloop/service/EventPersistenceSubscriber.kt +++ b/app/src/main/java/com/pulseloop/service/EventPersistenceSubscriber.kt @@ -7,6 +7,7 @@ import com.pulseloop.data.dao.MeasurementDeletionDao import com.pulseloop.data.entity.* import com.pulseloop.health.HealthConnectExportWorker import com.pulseloop.settings.QuietHoursPrefs +import com.pulseloop.settings.StepSourcePrefs import com.pulseloop.ring.* import kotlinx.coroutines.* @@ -45,6 +46,40 @@ class EventPersistenceSubscriber( private val scope = CoroutineScope(Dispatchers.IO + SupervisorJob()) private var job: Job? = null + /** The user's step-source preference. Plain SharedPreferences under the hood — cheap + * enough to read directly on every activity event, so no in-memory cache is needed. */ + private val stepSourcePrefs = StepSourcePrefs(context) + + /** + * Millis of the last successful [PulseEvent.PhoneStepsUpdate] this subscriber saw — i.e. + * the last time the phone reader actually managed to publish a value. 0 until the first + * successful read after process start, and reset to 0 on + * [PulseEvent.PhoneStepSourceUnavailable]. + */ + private var lastPhoneStepAt: Long = 0L + + /** + * True when the phone source should win over the ring. Requires **both** the user's + * preference and recent evidence the phone reader is working — a successful + * [PulseEvent.PhoneStepsUpdate] within [PHONE_STEP_GRACE_MS]. + * + * Without the freshness check, a revoked READ_STEPS, a removed HC provider, or a device + * with no step writer would leave the preference stuck on "phone" while + * [com.pulseloop.PhoneStepManager.refresh] silently failed, dropping the ring's live + * totals and freezing today's count with no path back. The grace period covers a long + * gap between app opens while still catching a real failure within a day. + * + * Read on every ring activity event, so the check is deliberately non-suspend and + * allocation-free — a plain timestamp comparison. + */ + private val stepSourceIsPhone: Boolean + get() { + if (stepSourcePrefs.stepSource != StepSourcePrefs.SOURCE_PHONE) return false + val last = lastPhoneStepAt + if (last == 0L) return false + return System.currentTimeMillis() - last < PHONE_STEP_GRACE_MS + } + // Battery-history throttle (iOS #61b) — in-memory, so the first reading after each (re)launch // always records; a change or a 30-min floor logs a fresh row otherwise, keeping the table to a // few dozen rows/day instead of one per BLE battery read. @@ -84,6 +119,7 @@ class EventPersistenceSubscriber( is PulseEvent.TemperatureSample, is PulseEvent.ActivityUpdate, is PulseEvent.ActivityBucket, + is PulseEvent.PhoneStepsUpdate, is PulseEvent.SleepTimeline -> true else -> false } @@ -381,7 +417,11 @@ class EventPersistenceSubscriber( else db.measurementDao().insert(measurement) } is PulseEvent.ActivityUpdate -> { - upsertActivityDaily(event.timestamp.toEpochMilli(), event.steps, event.calories, event.distanceMeters) + // When the user picks the phone as their step source, ignore the ring's + // live activity totals so its over-counted steps don't land in the DB. + if (!stepSourceIsPhone) { + upsertActivityDaily(event.timestamp.toEpochMilli(), event.steps, event.calories, event.distanceMeters) + } } is PulseEvent.ActivityBucket -> { // Per-slice ring history: upserted by timestamp + the day total recomputed as the @@ -390,6 +430,18 @@ class EventPersistenceSubscriber( // its single largest bucket. Calories omitted (unverified ring field). applyActivityBucket(event.timestamp.toEpochMilli(), event.steps, event.distanceMeters) } + is PulseEvent.PhoneStepsUpdate -> { + // Fresh phone value — refresh the persisted health timestamp that gates the + // ring fallback (see [stepSourceIsPhone]). + lastPhoneStepAt = System.currentTimeMillis() + upsertActivityDailyFromPhone(event.timestamp.toEpochMilli(), event.steps) + } + is PulseEvent.PhoneStepSourceUnavailable -> { + // A5: the phone reader just failed. Reset the health marker so [stepSourceIsPhone] + // stops gating the ring's live totals off; the ring fills in until the + // phone reader recovers on a later foreground. + lastPhoneStepAt = 0L + } is PulseEvent.SleepTimeline -> { upsertSleepSession(event.timestamp.toEpochMilli(), event.stages, event.completeSession) } @@ -496,15 +548,20 @@ class EventPersistenceSubscriber( val deletion = com.pulseloop.data.ActivityBucketDeletion val hasDeletion = existing.deletedSteps > 0 || existing.deletedDistanceMeters > 0.0 db.activityDailyDao().upsert(existing.copy( + // A4: if the row was previously marked "phone" and the ring is now writing + // (this path only runs when [stepSourceIsPhone] is false), reset the marker so + // the row's source reflects its current owner. Other sources + // ("ring_history", "manual_recording", "hr_and_manual") are preserved. + source = if (existing.source == StepSourcePrefs.SOURCE_PHONE) StepSourcePrefs.SOURCE_RING else existing.source, steps = if (stale) steps - else deletion.ratchetAgainstRing(existing.steps, steps, existing.deletedSteps), + else deletion.ratchetAgainstRing(existing.steps, steps, existing.deletedSteps), // A bucket carries no calorie field, so there is nothing to subtract from the // ring's own figure — the deletion drops it and the app's estimate takes over // (see [com.pulseloop.data.ActivityBucketDeletion]). Ratcheting here would put it // straight back. calories = if (stale) calories - else if (hasDeletion) existing.calories - else maxOf(existing.calories, calories), + else if (hasDeletion) existing.calories + else maxOf(existing.calories, calories), distanceMeters = if (stale) distanceM else deletion.ratchetAgainstRing( existing.distanceMeters, distanceM, existing.deletedDistanceMeters, ), @@ -513,11 +570,52 @@ class EventPersistenceSubscriber( } else { db.activityDailyDao().upsert(ActivityDailyEntity( date = dayStart, steps = steps, calories = calories, - distanceMeters = distanceM, source = "ring", + distanceMeters = distanceM, source = StepSourcePrefs.SOURCE_RING, )) } } + /** + * Persist the phone's step count for one day. + * + * **Overwrites** `steps` and `source` unconditionally. Distance is derived from the + * user's height (stride ≈ 0.414 × height, the same model + * [DailyCalorieEstimator.intermittentWalkMET] uses; falls back to 0.7 m/step when + * height is unknown). Calories are left at 0.0 so that + * [DailyCalorieEstimator.deviceReportedCalories] returns null and the display falls + * through to the estimator's own figure — which is the whole point of not writing + * `steps * 0.04` here. + * + * A no-op when the day already holds this exact phone value: on every foreground the + * phone reader re-publishes today's total, and without the guard every pass would touch + * `updatedAt` (which ActivityExporter selects by) and re-export the same HC record. + */ + private suspend fun upsertActivityDailyFromPhone(ts: Long, steps: Int) { + val dayStart = com.pulseloop.util.TimeUtil.startOfDayLocal(ts) + val existing = db.activityDailyDao().byDay(dayStart) + + if (existing != null && existing.steps == steps && existing.source == StepSourcePrefs.SOURCE_PHONE) return + + val profile = db.userProfileDao().get() + val heightCm = profile?.heightCm + val strideMeters = if (heightCm != null && heightCm > 0) 0.414 * heightCm / 100.0 else 0.7 + val distanceMeters = steps * strideMeters + + val now = System.currentTimeMillis() + db.activityDailyDao().upsert( + (existing ?: ActivityDailyEntity(date = dayStart, source = StepSourcePrefs.SOURCE_PHONE)).copy( + steps = steps, + distanceMeters = distanceMeters, + // Clear any stale ring calories; the estimator writes the real figure to + // `estimatedActiveCalories` on the next sync. + calories = 0.0, + source = StepSourcePrefs.SOURCE_PHONE, + syncedAt = now, + updatedAt = now, + ) + ) + } + /** * Persist one intraday activity bucket from ring history (a Colmi `0x43` sample) and recompute * its day's total. The bucket is **upserted by its start time**, so re-syncing the same bucket @@ -560,15 +658,25 @@ class EventPersistenceSubscriber( val isToday = dayStart == com.pulseloop.util.TimeUtil.startOfTodayLocal() val stale = existing != null && existing.steps > 200_000 val ratchet = isToday && existing != null && !stale - db.activityDailyDao().upsert( - (existing ?: ActivityDailyEntity(date = dayStart, source = "ring_history")).copy( - steps = if (ratchet) maxOf(existing!!.steps, totalSteps) else totalSteps, - distanceMeters = if (ratchet) maxOf(existing!!.distanceMeters, totalDistance) else totalDistance, - source = if (ratchet) existing!!.source else "ring_history", - syncedAt = System.currentTimeMillis(), - updatedAt = System.currentTimeMillis(), + // A4: "owned" means the row was written by the phone AND the phone source is still + // currently winning (the same condition [stepSourceIsPhone] checks — user preference + // plus recent successful reads). If the user has switched back to Ring, or the phone + // reader has gone stale, the ring's bucket sum must be allowed to retake the day; + // otherwise a single toggle to Phone would suppress the ring's totals for those days + // permanently. + val dayOwnedByPhone = existing?.source == StepSourcePrefs.SOURCE_PHONE && stepSourceIsPhone + val skipDailyTotal = dayOwnedByPhone || (stepSourceIsPhone && isToday) + if (!skipDailyTotal) { + db.activityDailyDao().upsert( + (existing ?: ActivityDailyEntity(date = dayStart, source = "ring_history")).copy( + steps = if (ratchet) maxOf(existing!!.steps, totalSteps) else totalSteps, + distanceMeters = if (ratchet) maxOf(existing!!.distanceMeters, totalDistance) else totalDistance, + source = if (ratchet) existing!!.source else "ring_history", + syncedAt = System.currentTimeMillis(), + updatedAt = System.currentTimeMillis(), + ) ) - ) + } } private suspend fun upsertSleepSession(ts: Long, stages: List, completeSession: Boolean) { @@ -730,7 +838,7 @@ class EventPersistenceSubscriber( val matched: List> = segments.map { seg -> val best = available.maxByOrNull { overlap(seg.start, seg.end, it.startAt, it.endAt) } if (best != null && (overlap(seg.start, seg.end, best.startAt, best.endAt) > 0L || - best.startAt in seg.start..seg.end)) { + best.startAt in seg.start..seg.end)) { available.remove(best) seg to best } else { @@ -788,7 +896,7 @@ class EventPersistenceSubscriber( * Matches the official app's scoring. */ private fun computeSleepScore(deepMin: Int, totalMin: Int): Int? = - // Delegates since issue #78: SleepRecordDeletion restates deleted-from sessions with the + // Delegates since issue #78: SleepRecordDeletion restates deleted-from sessions with the // same banding, so one function owns it. sleepStageScore(deepMin, totalMin) @@ -825,6 +933,13 @@ class EventPersistenceSubscriber( /** How far back [spotReadings] is primed from the table on first use. */ const val SPOT_LOOKBACK_MS = 7L * 24 * 60 * 60_000 private const val MAX_SLEEP_TIMELINE_MINUTES = 24 * 60 + + /** + * How long the subscriber trusts a "phone" preference without a successful phone read + * before falling back to the ring (see [stepSourceIsPhone]). Covers a long gap + * between app opens while still catching a real failure within a day. + */ + private const val PHONE_STEP_GRACE_MS = 12L * 3_600_000L } } diff --git a/app/src/main/java/com/pulseloop/settings/ApiKeyStore.kt b/app/src/main/java/com/pulseloop/settings/ApiKeyStore.kt index a679e4ba..f54a1669 100644 --- a/app/src/main/java/com/pulseloop/settings/ApiKeyStore.kt +++ b/app/src/main/java/com/pulseloop/settings/ApiKeyStore.kt @@ -9,6 +9,9 @@ import com.pulseloop.service.GlucoseUnit * Ported from OpenAIKeychainStore in the iOS app. * Stores the OpenAI API key securely using EncryptedSharedPreferences * (Android equivalent of iOS Keychain). + * + * Non-sensitive UI preferences like `stepSource` live in [StepSourcePrefs] instead — the + * encryption layer is expensive enough to avoid using it for values that don't need it. */ class ApiKeyStore(context: Context) { private val masterKey = MasterKeys.getOrCreate(MasterKeys.AES256_GCM_SPEC) @@ -204,4 +207,4 @@ class ApiKeyStore(context: Context) { private const val KEY_LUNG_CONDITION = "has_known_lung_condition" private const val KEY_GLUCOSE_UNIT = "preferred_glucose_unit" } -} +} \ No newline at end of file diff --git a/app/src/main/java/com/pulseloop/settings/StepSourcePrefs.kt b/app/src/main/java/com/pulseloop/settings/StepSourcePrefs.kt new file mode 100644 index 00000000..78c11b24 --- /dev/null +++ b/app/src/main/java/com/pulseloop/settings/StepSourcePrefs.kt @@ -0,0 +1,48 @@ +package com.pulseloop.settings + +import android.content.Context + +/** + * The user's step-source preference ("ring" or "phone"). + * + * Deliberately uses plain (unencrypted) [android.content.SharedPreferences] rather than + * [ApiKeyStore]'s EncryptedSharedPreferences: this is a non-sensitive UI preference, and the + * encryption layer is expensive enough that callers reading it on every ring activity event + * previously needed an in-memory cache. Plain prefs are cheap to read, so the cache is gone. + * + * Also holds the phone-step backfill throttle: [lastBackfillDay] is the local-midnight epoch + * millis of the last successful `PhoneStepManager.refreshHistoricalDays()` run, so the app + * backfills once per day instead of on every foreground (see + * [com.pulseloop.MainActivity.refreshPhoneSteps]). + */ +class StepSourcePrefs(context: Context) { + private val prefs = context.applicationContext + .getSharedPreferences("pulseloop_prefs", Context.MODE_PRIVATE) + + var stepSource: String + get() = prefs.getString(KEY_STEP_SOURCE, SOURCE_RING) ?: SOURCE_RING + set(value) { prefs.edit().putString(KEY_STEP_SOURCE, value).apply() } + + /** + * Local-midnight epoch millis of the last day on which the phone step reader ran a + * successful 30-day backfill. 0 means "never run". Read/written only from + * [com.pulseloop.MainActivity.refreshPhoneSteps] — the Settings toggle intentionally + * does not consult it, because an explicit user action should always run. + */ + var lastBackfillDay: Long + get() = prefs.getLong(KEY_LAST_BACKFILL_DAY, 0L) + set(value) { prefs.edit().putLong(KEY_LAST_BACKFILL_DAY, value).apply() } + + companion object { + /** + * The two valid values of [stepSource]. Every call site that reads or writes the + * preference — or that compares an `activity_daily.source` value it wrote — refers + * to one of these rather than a bare `"ring"` / `"phone"` literal. + */ + const val SOURCE_RING = "ring" + const val SOURCE_PHONE = "phone" + + private const val KEY_STEP_SOURCE = "step_source" + private const val KEY_LAST_BACKFILL_DAY = "last_backfill_day" + } +} \ No newline at end of file diff --git a/app/src/main/java/com/pulseloop/ui/screens/ActivityScreen.kt b/app/src/main/java/com/pulseloop/ui/screens/ActivityScreen.kt index f65b6b2e..e5dee641 100644 --- a/app/src/main/java/com/pulseloop/ui/screens/ActivityScreen.kt +++ b/app/src/main/java/com/pulseloop/ui/screens/ActivityScreen.kt @@ -44,6 +44,7 @@ import com.pulseloop.data.entity.ActivitySessionEntity import com.pulseloop.settings.ApiKeyStore import com.pulseloop.settings.UnitConverter import com.pulseloop.settings.UnitSystem +import com.pulseloop.settings.StepSourcePrefs import com.pulseloop.ui.components.ActivityMeta import com.pulseloop.ui.components.ActivityRing import com.pulseloop.ui.components.ActivityRings @@ -79,6 +80,10 @@ fun ActivityScreen( // remember{}: the ApiKeyStore constructor does Keystore + encrypted-prefs I/O — far // too expensive to repeat on every recomposition of this state-collecting screen. val units = remember { ApiKeyStore(context) }.resolvedUnitSystem + // Read the step-source preference once per composition. Cheap enough to read + // unconditionally (plain SharedPreferences, no crypto), and re-read on every entry to + // the screen so a toggle made in Settings is reflected on return. + val stepSourcePrefs = remember { StepSourcePrefs(context) } val scope = rememberCoroutineScope() var pickerOpen by remember { mutableStateOf(false) } var historyOpen by remember { mutableStateOf(false) } @@ -230,13 +235,21 @@ fun ActivityScreen( // The blocks behind today's total, each removable (issue #70). Under the summary rather // than beside it: it answers "why is that number wrong", which is a question you only ask // after reading the number. - item { - ActivityRecordsCard( - viewModel = viewModel, - units = units, - dayStart = shownDayStart(state), - dayLabel = shownDayLabel(state), - ) + // + // Hidden when the step source is "Phone": the Records list is the ring's intraday + // bucket log, and while the phone owns the daily total, showing ring-derived blocks + // beneath that total reads as a bug — the sum of the records does not match the header + // above them, and deleting one has no effect on a total the ring no longer owns. + // Toggling back to "Ring" restores the card. + if (stepSourcePrefs.stepSource != com.pulseloop.settings.StepSourcePrefs.SOURCE_PHONE) { + item { + ActivityRecordsCard( + viewModel = viewModel, + units = units, + dayStart = shownDayStart(state), + dayLabel = shownDayLabel(state), + ) + } } item { Spacer(Modifier.height(64.dp)) } } @@ -363,6 +376,12 @@ private fun DailyActivitySummaryCard(state: ActivityViewModel.ActivityState, uni // WeeklyGoalCard keeps reading state.today — a week widget is about the live week. val today = state.daySummary val distValue = today?.distanceMeters?.let { Formats.distance(UnitConverter.distance(it, units)) } + // Read the effective calories from the ViewModel rather than the row's raw `calories` + // column: the estimator's figure is what the Today card shows, and reading the column + // here would surface a phone row's stale hardcoded placeholder (or a ring's pre-toggle + // value) as if it were a real total. Same read path as TodayViewModel's, so the two + // screens cannot disagree. + val calories = state.effectiveCalories Row( Modifier .fillMaxWidth() @@ -379,14 +398,14 @@ private fun DailyActivitySummaryCard(state: ActivityViewModel.ActivityState, uni SummaryMetric("Distance", distValue ?: "—", if (distValue != null) UnitConverter.distanceUnit(units) else null, PulseColors.distance, Modifier.weight(1f)) } // The same figure the Today screen shows, not the row's raw `calories` (issue #101). - val calories = state.dayCalories + val calories = state.effectiveCalories SummaryMetric("Calories", calories?.let { Formats.count(it.toInt()) } ?: "—", if (calories != null) "cal" else null, PulseColors.calories) } ActivityRings( rings = listOf( ActivityRing(today?.steps?.toDouble(), state.stepGoal.toDouble(), PulseColors.steps), ActivityRing(today?.distanceMeters, state.distanceGoalMeters, PulseColors.distance), - ActivityRing(state.dayActiveCalories, state.caloriesGoal.toDouble(), PulseColors.calories), + ActivityRing(calories, state.caloriesGoal.toDouble(), PulseColors.calories), ), size = 112.dp, strokeWidth = 11.dp, diff --git a/app/src/main/java/com/pulseloop/ui/screens/SettingsScreen.kt b/app/src/main/java/com/pulseloop/ui/screens/SettingsScreen.kt index 9c1141ac..0d6bdb20 100644 --- a/app/src/main/java/com/pulseloop/ui/screens/SettingsScreen.kt +++ b/app/src/main/java/com/pulseloop/ui/screens/SettingsScreen.kt @@ -1,5 +1,6 @@ package com.pulseloop.ui.screens +import androidx.activity.compose.rememberLauncherForActivityResult import androidx.compose.foundation.layout.* import androidx.compose.foundation.rememberScrollState import androidx.compose.foundation.verticalScroll @@ -11,15 +12,22 @@ import androidx.compose.ui.Modifier import androidx.compose.ui.platform.LocalContext import androidx.compose.ui.text.font.FontWeight import androidx.compose.ui.unit.dp +import android.widget.Toast import com.pulseloop.coach.config.CoachProviderMode import com.pulseloop.coach.config.CoachProviderSettingsStore +import com.pulseloop.health.HealthConnectAvailability +import com.pulseloop.health.HealthConnectSdk import com.pulseloop.data.PulseLoopDatabase import com.pulseloop.ring.WearableCapability import com.pulseloop.settings.ApiKeyStore +import com.pulseloop.settings.StepSourcePrefs import com.pulseloop.ui.components.DeviceHeroCard import com.pulseloop.ui.components.SettingsRowItem import com.pulseloop.ui.components.SettingsSection import com.pulseloop.ui.theme.PulseColors +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.launch +import kotlinx.coroutines.withContext /** * Ported from SettingsView.swift (iOS #49 rehaul). @@ -35,8 +43,10 @@ fun SettingsScreen( ) { val context = LocalContext.current val keyStore = remember { ApiKeyStore(context) } + val stepSourcePrefs = remember { StepSourcePrefs(context) } val providerStore = remember { CoachProviderSettingsStore(context) } val db = remember { PulseLoopDatabase.getInstance(context) } + val scope = rememberCoroutineScope() val bleState = bleClient?.state?.collectAsState()?.value ?: com.pulseloop.ring.RingBLEClient.BLEState() @@ -51,6 +61,7 @@ fun SettingsScreen( // recomposes this hub, so toggles made there are reflected immediately. val coachEnabled = keyStore.coachEnabled val developerUnlocked = keyStore.developerUnlocked + var stepSource by remember { mutableStateOf(stepSourcePrefs.stepSource) } // Provider-aware AI Coach summary — mirrors iOS `coachTrailing` (no Apple on-device // mode on Android; hosted providers show the selected model slug). @@ -67,6 +78,35 @@ fun SettingsScreen( } val notificationsTrailing = if (keyStore.notificationsEnabled) "On" else "Off" + // The Health Connect READ_STEPS permission is requested only from the Step Source + // row below, on the user's explicit action. MainActivity deliberately does not + // prompt for it on launch or on foreground return — this is the single entry + // point, so a user who declines is not asked again until they touch the toggle + // once more. If the dialog is declined, the toggle reverts to "Ring" so the UI + // never claims a source the app cannot actually read from. + val healthConnectPermissionLauncher = rememberLauncherForActivityResult( + androidx.health.connect.client.PermissionController.createRequestPermissionResultContract() + ) { granted -> + if (granted.isNotEmpty()) { + // Granted — now it is safe to persist "phone" and run the backfill. A6: the + // preference is written only after permission is confirmed, so an interrupted + // flow cannot leave the app claiming a source it can't read from. + stepSourcePrefs.stepSource = StepSourcePrefs.SOURCE_PHONE + scope.launch(Dispatchers.IO) { + try { + com.pulseloop.PhoneStepManager(context).refreshHistoricalDays() + } catch (_: Exception) { + // A failed backfill just leaves the ring's data in place for + // historical days. Nothing user-visible to report. + } + } + } else { + // Declined. Do NOT persist "phone". Revert the UI toggle so it doesn't claim + // a source the app cannot read from. + stepSource = StepSourcePrefs.SOURCE_RING + } + } + fun navigate(route: String) { navController?.navigate(route) } @@ -141,6 +181,83 @@ fun SettingsScreen( ), ) + // STEP SOURCE — switching to "Phone" backfills the last 30 days of step totals + // from Health Connect, so past days show the phone's numbers rather than the + // ring's. The permission for Health Connect READ_STEPS is requested here, on the + // user's explicit action, and only when it isn't already granted. The backfill + // itself is fire-and-forget: the persistence layer receives PhoneStepsUpdate + // events and writes each day's row; the Activity screen's Flows pick up the new + // rows automatically. + SettingsSection( + title = "Step Source", + rows = listOf( + SettingsRowItem( + icon = Icons.Filled.Timeline, + tint = PulseColors.accent, + title = "Data Source", + trailingValue = if (stepSource == StepSourcePrefs.SOURCE_PHONE) "Phone" else "Ring" + ) { + val newSource = if (stepSource == StepSourcePrefs.SOURCE_RING) StepSourcePrefs.SOURCE_PHONE else StepSourcePrefs.SOURCE_RING + stepSource = newSource // optimistic UI; persisted only once proven valid + if (newSource == StepSourcePrefs.SOURCE_RING) { + // Ring is always safe — no external dependency to verify. + stepSourcePrefs.stepSource = StepSourcePrefs.SOURCE_RING + } else { + // Switching to Phone. A6: verify HC availability and permission + // BEFORE persisting. The old flow saved "phone" first and reverted + // on failure, so an interrupted run left the app claiming a source + // it couldn't read from, which then dropped the ring's live totals. + scope.launch { + val availability = withContext(Dispatchers.IO) { + HealthConnectSdk.availability(context) + } + if (availability != HealthConnectAvailability.AVAILABLE) { + Toast.makeText( + context, + when (availability) { + HealthConnectAvailability.PROVIDER_UPDATE_REQUIRED -> + "Health Connect needs an update before phone steps can be used." + else -> + "Health Connect isn't available on this device." + }, + Toast.LENGTH_LONG, + ).show() + stepSource = StepSourcePrefs.SOURCE_RING // revert the optimistic flip + return@launch + } + val readSteps = androidx.health.connect.client.permission.HealthPermission + .getReadPermission(androidx.health.connect.client.records.StepsRecord::class) + val hasPermission = withContext(Dispatchers.IO) { + try { + val client = androidx.health.connect.client.HealthConnectClient + .getOrCreate(context) + readSteps in client.permissionController.getGrantedPermissions() + } catch (_: Exception) { + false + } + } + if (hasPermission) { + // Already granted — persist and backfill now. + stepSourcePrefs.stepSource = StepSourcePrefs.SOURCE_PHONE + withContext(Dispatchers.IO) { + try { + com.pulseloop.PhoneStepManager(context).refreshHistoricalDays() + } catch (_: Exception) { + // A failed backfill just leaves the ring's data + // in place for historical days. + } + } + } else { + // Not granted — ask. The launcher's callback persists + // "phone" only once permission is confirmed. + healthConnectPermissionLauncher.launch(setOf(readSteps)) + } + } + } + } + ), + ) + // RESOURCES — Calibration is jring-only (Colmi rings measure neither BP nor blood // sugar); Developer is hidden until unlocked by tapping the version 7× in About. SettingsSection( @@ -175,4 +292,4 @@ fun SettingsScreen( Spacer(Modifier.height(32.dp)) } -} +} \ No newline at end of file diff --git a/app/src/main/java/com/pulseloop/ui/screens/TodayScreen.kt b/app/src/main/java/com/pulseloop/ui/screens/TodayScreen.kt index 7ea4cf21..5e7fc189 100644 --- a/app/src/main/java/com/pulseloop/ui/screens/TodayScreen.kt +++ b/app/src/main/java/com/pulseloop/ui/screens/TodayScreen.kt @@ -15,10 +15,12 @@ import androidx.compose.ui.text.font.FontWeight import androidx.compose.ui.text.style.TextOverflow import androidx.compose.ui.unit.dp import androidx.compose.ui.unit.sp +import com.pulseloop.PhoneStepManager import com.pulseloop.service.MetricKind import com.pulseloop.service.MetricZone import com.pulseloop.service.VitalsThresholdEngine import com.pulseloop.settings.ApiKeyStore +import com.pulseloop.settings.StepSourcePrefs import com.pulseloop.settings.UnitConverter import com.pulseloop.ui.components.* import com.pulseloop.ui.dashboard.CustomizeCardsButton @@ -66,6 +68,7 @@ fun TodayScreen( // remember{}: the ApiKeyStore constructor does Keystore + encrypted-prefs I/O — far // too expensive to repeat on every recomposition of this state-collecting screen. val keyStore = remember { ApiKeyStore(context) } + val stepSourcePrefs = remember { StepSourcePrefs(context) } val units = keyStore.resolvedUnitSystem val coachEnabled = keyStore.coachEnabled val pullRefreshState = rememberPullRefreshState( @@ -74,6 +77,12 @@ fun TodayScreen( isRefreshing = true scope.launch { coordinator?.pullToRefresh() + // Also refresh phone steps from Health Connect — otherwise pulling to + // refresh syncs the ring but leaves the activity tile showing a stale + // step count. No-op unless the user's step source is set to "Phone". + if (stepSourcePrefs.stepSource == StepSourcePrefs.SOURCE_PHONE) { + PhoneStepManager(context).refresh() + } kotlinx.coroutines.delay(1500) isRefreshing = false } @@ -112,8 +121,8 @@ fun TodayScreen( val visibleKeys = allSupported.filter { !prefs.isHidden(it, MetricScope.TODAY) }.map { it.key }.toSet() val visibleOrdered: List = run { val base = if (editState.editing) editState.liveOrder - else prefsStore.resolvedOrder(visibleKeys, DashboardCard.todayDefault.map { it.key }, MetricScope.TODAY) - .mapNotNull { DashboardCard.fromKey(it) } + else prefsStore.resolvedOrder(visibleKeys, DashboardCard.todayDefault.map { it.key }, MetricScope.TODAY) + .mapNotNull { DashboardCard.fromKey(it) } base.filter { it.key in visibleKeys } } val hiddenCards = allSupported.filter { prefs.isHidden(it, MetricScope.TODAY) } @@ -398,7 +407,7 @@ private fun deriveHero(state: TodayViewModel.TodayState, sleep: SleepViewModel.S val series = state.steps7d.map { it.toDouble() } val prior = series.dropLast(1) val base = if (prior.isNotEmpty() && prior.average() > 0) prior.average() - else if (series.isNotEmpty()) series.average() else 0.0 + else if (series.isNotEmpty()) series.average() else 0.0 val stepsDelta = if (base == 0.0) 0 else (((steps - base) / base) * 100).toInt() val title = when { diff --git a/app/src/main/java/com/pulseloop/ui/viewmodels/ViewModels.kt b/app/src/main/java/com/pulseloop/ui/viewmodels/ViewModels.kt index e8a79943..b12f35fe 100644 --- a/app/src/main/java/com/pulseloop/ui/viewmodels/ViewModels.kt +++ b/app/src/main/java/com/pulseloop/ui/viewmodels/ViewModels.kt @@ -450,13 +450,16 @@ class ActivityViewModel(db: PulseLoopDatabase) : ViewModel() { val shownDay: Long = 0, /** The shown day's totals (the summary card); `today` stays live for the week widget. */ val daySummary: ActivityDailyEntity? = null, - /** The shown day's calories as the Today screen shows them (issue #101): the ring's own - * figure when it reported one, else BMR accrued plus the active estimate. The row's raw - * `calories` stays 0 for rings that report none, which is why this exists. */ - val dayCalories: Double? = null, - /** The active-energy part of [dayCalories] — what the calorie goal ring measures, since the - * calorie goal is an active-energy goal ([DailyCalorieEstimator.effectiveActiveCalories]). */ - val dayActiveCalories: Double? = null, + /** + * Effective calories for the shown day: the ring's own figure when it reported one, + * else [DailyCalorieEstimator.effectiveCalories] (BMR accrued through the day so far + * plus the net active estimate). Mirrors [TodayViewModel]'s read path so the Today + * card and the Activity card cannot disagree about what a day's calories are — a + * `phone`-sourced row would otherwise show its raw `calories` column, which is either + * the estimator's own fill-in or the stale placeholder written before that path + * existed. + */ + val effectiveCalories: Double? = null, val stepGoal: Int = UserGoalEntity.DEFAULT_STEPS, val activeMinutesGoal: Int = 45, val distanceGoalMeters: Double = UserGoalEntity.DEFAULT_DISTANCE_METERS, @@ -495,6 +498,13 @@ class ActivityViewModel(db: PulseLoopDatabase) : ViewModel() { } viewModelScope.launch { shownDayStart.flatMapLatest { day -> db.activityDailyDao().byDayFlow(day) }.collect { day -> + // Effective calories for the shown day — same read path as the Today card + // (see TodayViewModel's init), so both screens agree. Reading `day.calories` + // straight off the row would surface the ring's value for ring modes, but + // for phone-sourced rows it would show the estimator's own fill-in — or, on + // older rows, the hardcoded `steps * 0.04` placeholder — as if it were a + // device-reported figure. [DailyCalorieEstimator.effectiveCalories] handles + // both cases: device figure when there is one, else BMR + active estimate. val profile = db.userProfileDao().get()?.let { DailyCalorieEstimator.Profile(it.sex, it.age, it.weightKg, it.heightCm) } @@ -502,8 +512,7 @@ class ActivityViewModel(db: PulseLoopDatabase) : ViewModel() { profile?.let { DailyCalorieEstimator.effectiveCalories(row, it) } ?: DailyCalorieEstimator.deviceReportedCalories(row) } - val activeCalories = day?.let { DailyCalorieEstimator.effectiveActiveCalories(it) } - _state.update { it.copy(daySummary = day, dayCalories = calories, dayActiveCalories = activeCalories) } + _state.update { it.copy(daySummary = day, effectiveCalories = calories) } } } viewModelScope.launch { From 9e03378536ea1d0743fe50df7d30317c9c28564b Mon Sep 17 00:00:00 2001 From: leona Date: Mon, 5 Oct 2026 16:03:46 +0200 Subject: [PATCH 02/33] Add regression tests for the phone step source --- .../pulseloop/service/PhoneStepSourceTest.kt | 50 +++++++++++++++++++ 1 file changed, 50 insertions(+) create mode 100644 app/src/test/java/com/pulseloop/service/PhoneStepSourceTest.kt diff --git a/app/src/test/java/com/pulseloop/service/PhoneStepSourceTest.kt b/app/src/test/java/com/pulseloop/service/PhoneStepSourceTest.kt new file mode 100644 index 00000000..b8be2c4c --- /dev/null +++ b/app/src/test/java/com/pulseloop/service/PhoneStepSourceTest.kt @@ -0,0 +1,50 @@ +package com.pulseloop.service + +import com.pulseloop.health.HealthConnectTypeMappings +import com.pulseloop.ring.PulseEvent +import com.pulseloop.settings.StepSourcePrefs +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNotEquals +import org.junit.Assert.assertTrue +import org.junit.Test +import java.time.Instant + +/** + * Regression guards for the phone step source. Deeper coverage of the Health Connect reads + * (`PhoneStepManager`) would need a mocked HC client and is out of scope here — these tests + * protect the invariants that are easy to break silently: + * + * - the exporter must never re-publish a phone row (that closes the self-feedback loop) + * - the two source constants stay distinct (a typo collapsing them would silently break the + * preference switch) + * - the event carries only the step count (adding a hardcoded distance/calorie back would + * bypass DailyCalorieEstimator without any compile error) + */ +class PhoneStepSourceTest { + + @Test + fun excludedSourcesContainsPhone() { + assertTrue( + "EXCLUDED_SOURCES must contain \"phone\" so ActivityExporter never writes a " + + "phone-sourced row to Health Connect — that write is the self-feedback loop.", + HealthConnectTypeMappings.EXCLUDED_SOURCES.contains(StepSourcePrefs.SOURCE_PHONE), + ) + } + + @Test + fun sourceConstantsAreDistinct() { + assertNotEquals(StepSourcePrefs.SOURCE_RING, StepSourcePrefs.SOURCE_PHONE) + } + + @Test + fun phoneStepsUpdateCarriesOnlySteps() { + // Constructing the event with only (timestamp, steps) is itself the regression guard: + // if anyone reintroduces `distanceMeters` or `calories` as required constructor + // parameters, this file stops compiling. + val event = PulseEvent.PhoneStepsUpdate( + timestamp = Instant.ofEpochMilli(0L), + steps = 123, + ) + assertEquals(123, event.steps) + } +} From a9ee09587b5fe49b9fd967e9fc5b757171ed6793 Mon Sep 17 00:00:00 2001 From: leona Date: Tue, 6 Oct 2026 21:50:04 +0200 Subject: [PATCH 03/33] Add phoneSteps column to ActivityDailyEntity + Room migration --- .../com/pulseloop/data/PulseLoopDatabase.kt | 18 +++++++++++++++++- .../com/pulseloop/data/entity/CoreEntities.kt | 5 +++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/app/src/main/java/com/pulseloop/data/PulseLoopDatabase.kt b/app/src/main/java/com/pulseloop/data/PulseLoopDatabase.kt index f09ba2b6..5e527811 100644 --- a/app/src/main/java/com/pulseloop/data/PulseLoopDatabase.kt +++ b/app/src/main/java/com/pulseloop/data/PulseLoopDatabase.kt @@ -44,7 +44,7 @@ import com.pulseloop.data.entity.* CachedFoodProductEntity::class, MeasurementDeletionEntity::class, ], - version = 28, + version = 29, exportSchema = false, ) abstract class PulseLoopDatabase : RoomDatabase() { @@ -523,6 +523,21 @@ abstract class PulseLoopDatabase : RoomDatabase() { } } + /** + * v28 -> v29: `activity_daily.phoneSteps` — the phone-source step column. + * + * Phone-sourced steps live in their own column instead of overwriting [ActivityDailyEntity.steps], + * so the ring's totals (steps, calories, distance) are never clobbered and switching the step + * source is a read-time choice, not a destructive write. Null for every existing row: no day + * has a phone value under the new design yet, and the display falls back to `steps` when null. + */ + private val MIGRATION_28_29 = object : Migration(28, 29) { + override fun migrate(db: SupportSQLiteDatabase) { + db.execSQL("ALTER TABLE `activity_daily` ADD COLUMN `phoneSteps` INTEGER") + } + } + + private fun adoptStableMeasurementIdentities(db: SupportSQLiteDatabase) { db.execSQL("DROP INDEX IF EXISTS `index_measurements_kindRaw_timestamp_sourceRaw`") db.execSQL( @@ -617,6 +632,7 @@ abstract class PulseLoopDatabase : RoomDatabase() { MIGRATION_25_26, MIGRATION_26_27, MIGRATION_27_28, + MIGRATION_28_29, ) // Downgrades only (sideloading an older APK). A blanket destructive // fallback would silently wipe every measurement, sleep session, and diff --git a/app/src/main/java/com/pulseloop/data/entity/CoreEntities.kt b/app/src/main/java/com/pulseloop/data/entity/CoreEntities.kt index 6e74fd85..93d3d86d 100644 --- a/app/src/main/java/com/pulseloop/data/entity/CoreEntities.kt +++ b/app/src/main/java/com/pulseloop/data/entity/CoreEntities.kt @@ -77,6 +77,11 @@ data class ActivityDailyEntity( @PrimaryKey val id: String = java.util.UUID.randomUUID().toString(), val date: Long, // epoch millis, start of day val steps: Int = 0, + /** Phone-sourced step count for this day (Health Connect), stored separately from [steps]. + * Null = no phone value written for this day. The display picks between [steps] and + * [phoneSteps] at read time based on the user's step-source preference; neither column + * overwrites the other, so switching source is reversible. */ + val phoneSteps: Int? = null, val calories: Double = 0.0, val distanceMeters: Double = 0.0, val activeMinutes: Int = 0, From 361eb61d477c92f4e6991945ae631e13d38e43b0 Mon Sep 17 00:00:00 2001 From: leona Date: Tue, 6 Oct 2026 22:04:36 +0200 Subject: [PATCH 04/33] Phone step write: write only phoneSteps, leave ring columns untouched --- .../service/EventPersistenceSubscriber.kt | 49 ++++++++++--------- 1 file changed, 26 insertions(+), 23 deletions(-) diff --git a/app/src/main/java/com/pulseloop/service/EventPersistenceSubscriber.kt b/app/src/main/java/com/pulseloop/service/EventPersistenceSubscriber.kt index 92fe4d97..a12a14d8 100644 --- a/app/src/main/java/com/pulseloop/service/EventPersistenceSubscriber.kt +++ b/app/src/main/java/com/pulseloop/service/EventPersistenceSubscriber.kt @@ -576,40 +576,43 @@ class EventPersistenceSubscriber( } /** - * Persist the phone's step count for one day. + * Persist the phone's step count for one day, in its own column. * - * **Overwrites** `steps` and `source` unconditionally. Distance is derived from the - * user's height (stride ≈ 0.414 × height, the same model - * [DailyCalorieEstimator.intermittentWalkMET] uses; falls back to 0.7 m/step when - * height is unknown). Calories are left at 0.0 so that - * [DailyCalorieEstimator.deviceReportedCalories] returns null and the display falls - * through to the estimator's own figure — which is the whole point of not writing - * `steps * 0.04` here. + * Under the separate-column design ([ActivityDailyEntity.phoneSteps]), an existing row is + * updated in **one column only**: `phoneSteps`. `steps`, `distanceMeters`, `calories`, and + * `source` are left exactly as the ring wrote them. The display picks between `steps` and + * `phoneSteps` at read time based on the user's step-source preference, so switching source + * is a read-time choice, not a destructive write: the ring's totals are never clobbered and + * switching back is instant. * - * A no-op when the day already holds this exact phone value: on every foreground the - * phone reader re-publishes today's total, and without the guard every pass would touch - * `updatedAt` (which ActivityExporter selects by) and re-export the same HC record. + * A brand-new row (phone wrote first, ring hasn't yet) is created with + * `source = "phone"` for two reasons: + * - so the day is visible in the "real" DAO queries + * (`source NOT IN ('demo','mock')`) even before the ring has data for it, and + * - so the row — which has `steps = 0` — stays out of the Health Connect export via + * [com.pulseloop.health.HealthConnectTypeMappings.EXCLUDED_SOURCES]. The old design + * needed that exclusion to break a self-feedback loop (the phone's value used to land + * in `steps`, the exported column); under the separate-column design the loop is broken + * by construction, and the exclusion is now only about not exporting a 0-step record. + * + * Later ring writes preserve that source — see [upsertActivityDaily]. The old "overwrite + * source and reset it back to ring on next ring write" dance is gone; nothing needs it any + * more. + * + * A no-op when the day already holds this exact phone value: on every foreground the phone + * reader re-publishes today's total, and without the guard every pass would touch `updatedAt` + * (which ActivityExporter selects by) and re-export the same HC record. */ private suspend fun upsertActivityDailyFromPhone(ts: Long, steps: Int) { val dayStart = com.pulseloop.util.TimeUtil.startOfDayLocal(ts) val existing = db.activityDailyDao().byDay(dayStart) - if (existing != null && existing.steps == steps && existing.source == StepSourcePrefs.SOURCE_PHONE) return - - val profile = db.userProfileDao().get() - val heightCm = profile?.heightCm - val strideMeters = if (heightCm != null && heightCm > 0) 0.414 * heightCm / 100.0 else 0.7 - val distanceMeters = steps * strideMeters + if (existing != null && existing.phoneSteps == steps) return val now = System.currentTimeMillis() db.activityDailyDao().upsert( (existing ?: ActivityDailyEntity(date = dayStart, source = StepSourcePrefs.SOURCE_PHONE)).copy( - steps = steps, - distanceMeters = distanceMeters, - // Clear any stale ring calories; the estimator writes the real figure to - // `estimatedActiveCalories` on the next sync. - calories = 0.0, - source = StepSourcePrefs.SOURCE_PHONE, + phoneSteps = steps, syncedAt = now, updatedAt = now, ) From 22ba4689ffbf78ec1059e19ab33459bf25e8ef79 Mon Sep 17 00:00:00 2001 From: leona Date: Tue, 6 Oct 2026 22:33:11 +0200 Subject: [PATCH 05/33] Read path: pick phoneSteps vs steps at display time by user preference --- .../java/com/pulseloop/ui/PulseLoopApp.kt | 11 +++- .../com/pulseloop/ui/viewmodels/ViewModels.kt | 60 ++++++++++++++++--- 2 files changed, 60 insertions(+), 11 deletions(-) diff --git a/app/src/main/java/com/pulseloop/ui/PulseLoopApp.kt b/app/src/main/java/com/pulseloop/ui/PulseLoopApp.kt index 724a8a69..ed10000d 100644 --- a/app/src/main/java/com/pulseloop/ui/PulseLoopApp.kt +++ b/app/src/main/java/com/pulseloop/ui/PulseLoopApp.kt @@ -126,13 +126,18 @@ fun PulseLoopApp() { } // ── ViewModels ─────────────────────────────────────────────────── - val todayVM = remember { TodayViewModel(db, apiKeyStore) } + val stepSourcePrefs = remember { com.pulseloop.settings.StepSourcePrefs(context) } + val todayVM = remember { TodayViewModel(db, apiKeyStore, stepSourcePrefs) } val vitalsVM = remember { VitalsViewModel(db, apiKeyStore) } val sleepVM = remember { val appContext = context.applicationContext - SleepViewModel(db, gapBridge = { com.pulseloop.service.SleepGapBridge.fromSettings(appContext) }) + SleepViewModel( + db, + gapBridge = { com.pulseloop.service.SleepGapBridge.fromSettings(appContext) }, + stepSourcePrefs = stepSourcePrefs, + ) } - val activityVM = remember { ActivityViewModel(db) } + val activityVM = remember { ActivityViewModel(db, stepSourcePrefs) } val weatherContextService = remember { com.pulseloop.coach.context.WeatherContextService(context) } val coachVM = remember { CoachViewModel( diff --git a/app/src/main/java/com/pulseloop/ui/viewmodels/ViewModels.kt b/app/src/main/java/com/pulseloop/ui/viewmodels/ViewModels.kt index b12f35fe..e1a5ab3f 100644 --- a/app/src/main/java/com/pulseloop/ui/viewmodels/ViewModels.kt +++ b/app/src/main/java/com/pulseloop/ui/viewmodels/ViewModels.kt @@ -23,6 +23,7 @@ import com.pulseloop.service.SleepScoreResult import com.pulseloop.service.UserPhysiologyProfile import com.pulseloop.service.VitalSample import com.pulseloop.settings.ApiKeyStore +import com.pulseloop.settings.StepSourcePrefs import com.pulseloop.settings.UnitConverter import com.pulseloop.settings.UnitSystem import com.pulseloop.ui.components.MetricThresholds @@ -34,12 +35,42 @@ import kotlinx.coroutines.launch import java.time.Instant import java.time.ZoneId +/** + * The user's step-source preference, read at each DB emission rather than observed as a + * Flow: plain SharedPreferences reads are cheap, and this matches how + * [com.pulseloop.service.EventPersistenceSubscriber] reads the same preference. + */ +private fun StepSourcePrefs.preferPhone(): Boolean = stepSource == StepSourcePrefs.SOURCE_PHONE + +/** + * The step count a screen should show for this day under the user's step-source preference + * (Path B read-time selection, PR #98). Ring mode shows the ring's own column; phone mode shows + * [ActivityDailyEntity.phoneSteps] when the phone has a value for the day, falling back to the + * ring's column when it does not — so a day the phone never backfilled still shows something. + */ +private fun ActivityDailyEntity.displaySteps(preferPhone: Boolean): Int = + if (preferPhone) phoneSteps ?: steps else steps + +/** + * The whole entity a screen should show, with `steps` projected for display under the user's + * step-source preference. Only `steps` is rewritten; `distanceMeters`, `calories`, and the rest + * of the row are left as the ring wrote them (Path B — see PR #98's design notes). Callers that + * hold a whole row in their state ([ActivityViewModel]) use this; callers that only need the + * scalar use [displaySteps]. + */ +private fun ActivityDailyEntity.forDisplay(preferPhone: Boolean): ActivityDailyEntity = + if (preferPhone && phoneSteps != null) copy(steps = phoneSteps!!) else this + /** * TodayViewModel — reads Room data for the Today dashboard. * Ported from MetricsService.buildTodaySummary in PulseServices.swift. * Uses reactive Flow queries so live ring data appears immediately. */ -class TodayViewModel(db: PulseLoopDatabase, private val apiKeyStore: ApiKeyStore? = null) : ViewModel() { +class TodayViewModel( + db: PulseLoopDatabase, + private val apiKeyStore: ApiKeyStore? = null, + private val stepSourcePrefs: StepSourcePrefs, +) : ViewModel() { // Local midnight, not UTC — the Today dashboard rolls over at the device's local // midnight so daily stats line up with how the rest of the app keys per-day rows. private val todayStart = MutableStateFlow(TimeUtil.startOfTodayLocal()) @@ -98,7 +129,7 @@ class TodayViewModel(db: PulseLoopDatabase, private val apiKeyStore: ApiKeyStore ?: DailyCalorieEstimator.deviceReportedCalories(row) } _state.update { it.copy( - steps = activity?.steps, + steps = activity?.displaySteps(stepSourcePrefs.preferPhone()), calories = calories, distanceMeters = activity?.distanceMeters, activeMinutes = activity?.activeMinutes, @@ -121,7 +152,7 @@ class TodayViewModel(db: PulseLoopDatabase, private val apiKeyStore: ApiKeyStore // 7-day step series for the hero delta (oldest→newest; DAO returns newest-first). viewModelScope.launch { db.activityDailyDao().recentFlow(7).collect { days -> - _state.update { it.copy(steps7d = days.reversed().map { d -> d.steps }) } + _state.update { it.copy(steps7d = days.reversed().map { d -> d.displaySteps(stepSourcePrefs.preferPhone()) }) } } } // Today's coach summary card (kind="today", scopeKey=local date). @@ -208,6 +239,7 @@ class SleepViewModel( private val db: PulseLoopDatabase, /** The night-bridging rule a record delete/edit re-segments with — the sync's own (issue #105). */ private val gapBridge: () -> com.pulseloop.service.SleepGapBridge = { com.pulseloop.service.SleepGapBridge.NONE }, + private val stepSourcePrefs: StepSourcePrefs, ) : ViewModel() { data class SleepState( val range: SleepRangeKey = SleepRangeKey.DAY, @@ -271,7 +303,10 @@ class SleepViewModel( blocksCache.getOrPut(id) { db.sleepStageBlockDao().forSession(id) } // aggregateCoach/averageScore take a synchronous lookup; pre-warm the cache first. val goal = try { db.userGoalDao().get()?.sleepMinutes } catch (_: Exception) { null } - val steps = try { db.activityDailyDao().byDay(TimeUtil.startOfTodayLocal())?.steps } catch (_: Exception) { null } + val steps = try { + db.activityDailyDao().byDay(TimeUtil.startOfTodayLocal()) + ?.displaySteps(stepSourcePrefs.preferPhone()) + } catch (_: Exception) { null } // ── Today tile (never moves with Day-view navigation) ── val todayReference = TimeUtil.referenceNightLocal() @@ -434,7 +469,10 @@ class SleepViewModel( * ActivityViewModel — reads Room data for the Activity screen. */ @OptIn(kotlinx.coroutines.ExperimentalCoroutinesApi::class) -class ActivityViewModel(db: PulseLoopDatabase) : ViewModel() { +class ActivityViewModel( + db: PulseLoopDatabase, + private val stepSourcePrefs: StepSourcePrefs, +) : ViewModel() { data class ActivityState( val recentDays: List = emptyList(), /** All finished sessions, newest first (drives Today + the history sheet). */ @@ -488,12 +526,13 @@ class ActivityViewModel(db: PulseLoopDatabase) : ViewModel() { init { viewModelScope.launch { db.activityDailyDao().recentFlow(7).collect { days -> - _state.update { it.copy(recentDays = days) } + val preferPhone = stepSourcePrefs.preferPhone() + _state.update { it.copy(recentDays = days.map { d -> d.forDisplay(preferPhone) }) } } } viewModelScope.launch { currentDayValues(todayStart, db.activityDailyDao()::byDayFlow).collect { day -> - _state.update { it.copy(today = day) } + _state.update { it.copy(today = day?.forDisplay(stepSourcePrefs.preferPhone())) } } } viewModelScope.launch { @@ -512,7 +551,12 @@ class ActivityViewModel(db: PulseLoopDatabase) : ViewModel() { profile?.let { DailyCalorieEstimator.effectiveCalories(row, it) } ?: DailyCalorieEstimator.deviceReportedCalories(row) } - _state.update { it.copy(daySummary = day, effectiveCalories = calories) } + _state.update { + it.copy( + daySummary = day?.forDisplay(stepSourcePrefs.preferPhone()), + effectiveCalories = calories, + ) + } } } viewModelScope.launch { From 03f7c515228cd812c28aa477c3d28ac89fecf953 Mon Sep 17 00:00:00 2001 From: leona Date: Tue, 6 Oct 2026 22:46:31 +0200 Subject: [PATCH 06/33] Read path: widgets and notifications honor the step-source preference --- .../com/pulseloop/data/entity/CoreEntities.kt | 22 +++++++++++++++- .../CoachNotificationSlotRunner.kt | 6 ++++- .../NotificationContextBuilder.kt | 10 +++++-- .../com/pulseloop/settings/StepSourcePrefs.kt | 8 ++++++ .../com/pulseloop/ui/viewmodels/ViewModels.kt | 26 ------------------- .../widgets/WidgetSnapshotPublisher.kt | 21 +++++++++++---- 6 files changed, 58 insertions(+), 35 deletions(-) diff --git a/app/src/main/java/com/pulseloop/data/entity/CoreEntities.kt b/app/src/main/java/com/pulseloop/data/entity/CoreEntities.kt index 93d3d86d..62797814 100644 --- a/app/src/main/java/com/pulseloop/data/entity/CoreEntities.kt +++ b/app/src/main/java/com/pulseloop/data/entity/CoreEntities.kt @@ -105,7 +105,27 @@ data class ActivityDailyEntity( */ val deletedSteps: Int = 0, val deletedDistanceMeters: Double = 0.0, -) +) { + /** + * The step count a screen should show for this day under the user's step-source preference + * (Path B read-time selection, PR #98). Ring mode shows the ring's own column; phone mode + * shows [phoneSteps] when the phone has a value for the day, falling back to the ring's + * column when it does not — so a day the phone never backfilled still shows something. + */ + fun displaySteps(preferPhone: Boolean): Int = + if (preferPhone) phoneSteps ?: steps else steps + + /** + * The whole entity a screen should show, with `steps` projected for display under the user's + * step-source preference. Only `steps` is rewritten; `distanceMeters`, `calories`, and the + * rest of the row are left as the ring wrote them (Path B — see PR #98's design notes). + * Callers that hold a whole row in their state (e.g. + * [com.pulseloop.ui.viewmodels.ActivityViewModel]) use this; callers that only need the + * scalar use [displaySteps]. + */ + fun forDisplay(preferPhone: Boolean): ActivityDailyEntity = + if (preferPhone && phoneSteps != null) copy(steps = phoneSteps) else this +} /** * Ported from [ActivityBucketSample] in PulseModels.swift. diff --git a/app/src/main/java/com/pulseloop/notifications/CoachNotificationSlotRunner.kt b/app/src/main/java/com/pulseloop/notifications/CoachNotificationSlotRunner.kt index 1c4e36cb..a47deb6b 100644 --- a/app/src/main/java/com/pulseloop/notifications/CoachNotificationSlotRunner.kt +++ b/app/src/main/java/com/pulseloop/notifications/CoachNotificationSlotRunner.kt @@ -458,7 +458,11 @@ private class CoachSlotProductionEngine( */ suspend fun generate(slot: CoachNotificationSlot, now: Long): CoachNotificationContent { val environment = WeatherContextService(context).snapshot() - val packet = NotificationContextBuilder.build(slot, db, now, environment = environment) + // Path B (PR #98): the check-in quotes the step count from the source the user chose. + val preferPhone = com.pulseloop.settings.StepSourcePrefs(context).preferPhone() + val packet = NotificationContextBuilder.build( + slot, db, now, environment = environment, preferPhone = preferPhone, + ) // Variety + anti-repeat (iOS #65): a deterministic per-day/slot coaching angle, // plus the last few delivered check-ins so the model doesn't repeat itself. diff --git a/app/src/main/java/com/pulseloop/notifications/NotificationContextBuilder.kt b/app/src/main/java/com/pulseloop/notifications/NotificationContextBuilder.kt index ab4a986c..9d2c9a80 100644 --- a/app/src/main/java/com/pulseloop/notifications/NotificationContextBuilder.kt +++ b/app/src/main/java/com/pulseloop/notifications/NotificationContextBuilder.kt @@ -23,6 +23,8 @@ object NotificationContextBuilder { db: PulseLoopDatabase, now: Long = System.currentTimeMillis(), environment: EnvironmentContext? = null, + /** When true, today's steps come from [ActivityDailyEntity.phoneSteps] (Path B, PR #98). */ + preferPhone: Boolean = false, ): NotificationContextPacket { val cutoff = now - 12 * 3600_000L @@ -30,9 +32,13 @@ object NotificationContextBuilder { val goal = db.userGoalDao().get() val device = db.deviceDao().current() - // Today's activity + // Today's activity. Steps are picked at read time by the user's step-source preference + // (Path B, PR #98) — a phone-mode evening check-in should quote the phone's count, not + // the ring's. Distance and calories stay as the ring wrote them, matching every other + // display read site. val todayStart = localStartOfDay(now) val todayActivity = db.activityDailyDao().byDay(todayStart) + val todaySteps = todayActivity?.displaySteps(preferPhone) // Latest sleep val latestSleep = db.sleepSessionDao().recent(1).firstOrNull() @@ -93,7 +99,7 @@ object NotificationContextBuilder { exerciseDaysWeekly = goal?.workoutsPerWeek ?: 4, ), today = NotificationContextPacket.DayContext( - steps = todayActivity?.steps, + steps = todaySteps, calories = todayActivity?.calories, distanceMeters = todayActivity?.distanceMeters, activeMinutes = todayActivity?.activeMinutes, diff --git a/app/src/main/java/com/pulseloop/settings/StepSourcePrefs.kt b/app/src/main/java/com/pulseloop/settings/StepSourcePrefs.kt index 78c11b24..94026c8f 100644 --- a/app/src/main/java/com/pulseloop/settings/StepSourcePrefs.kt +++ b/app/src/main/java/com/pulseloop/settings/StepSourcePrefs.kt @@ -23,6 +23,14 @@ class StepSourcePrefs(context: Context) { get() = prefs.getString(KEY_STEP_SOURCE, SOURCE_RING) ?: SOURCE_RING set(value) { prefs.edit().putString(KEY_STEP_SOURCE, value).apply() } + /** + * True when the user has chosen the phone as the step source. The read-time column selection + * (Path B, PR #98) branches on this: [ActivityDailyEntity.displaySteps] returns + * [ActivityDailyEntity.phoneSteps] when this is true and a phone value exists for the day, + * and [ActivityDailyEntity.steps] otherwise. + */ + fun preferPhone(): Boolean = stepSource == SOURCE_PHONE + /** * Local-midnight epoch millis of the last day on which the phone step reader ran a * successful 30-day backfill. 0 means "never run". Read/written only from diff --git a/app/src/main/java/com/pulseloop/ui/viewmodels/ViewModels.kt b/app/src/main/java/com/pulseloop/ui/viewmodels/ViewModels.kt index e1a5ab3f..980b99b3 100644 --- a/app/src/main/java/com/pulseloop/ui/viewmodels/ViewModels.kt +++ b/app/src/main/java/com/pulseloop/ui/viewmodels/ViewModels.kt @@ -35,32 +35,6 @@ import kotlinx.coroutines.launch import java.time.Instant import java.time.ZoneId -/** - * The user's step-source preference, read at each DB emission rather than observed as a - * Flow: plain SharedPreferences reads are cheap, and this matches how - * [com.pulseloop.service.EventPersistenceSubscriber] reads the same preference. - */ -private fun StepSourcePrefs.preferPhone(): Boolean = stepSource == StepSourcePrefs.SOURCE_PHONE - -/** - * The step count a screen should show for this day under the user's step-source preference - * (Path B read-time selection, PR #98). Ring mode shows the ring's own column; phone mode shows - * [ActivityDailyEntity.phoneSteps] when the phone has a value for the day, falling back to the - * ring's column when it does not — so a day the phone never backfilled still shows something. - */ -private fun ActivityDailyEntity.displaySteps(preferPhone: Boolean): Int = - if (preferPhone) phoneSteps ?: steps else steps - -/** - * The whole entity a screen should show, with `steps` projected for display under the user's - * step-source preference. Only `steps` is rewritten; `distanceMeters`, `calories`, and the rest - * of the row are left as the ring wrote them (Path B — see PR #98's design notes). Callers that - * hold a whole row in their state ([ActivityViewModel]) use this; callers that only need the - * scalar use [displaySteps]. - */ -private fun ActivityDailyEntity.forDisplay(preferPhone: Boolean): ActivityDailyEntity = - if (preferPhone && phoneSteps != null) copy(steps = phoneSteps!!) else this - /** * TodayViewModel — reads Room data for the Today dashboard. * Ported from MetricsService.buildTodaySummary in PulseServices.swift. diff --git a/app/src/main/java/com/pulseloop/widgets/WidgetSnapshotPublisher.kt b/app/src/main/java/com/pulseloop/widgets/WidgetSnapshotPublisher.kt index 1f6c9c70..a1f9c747 100644 --- a/app/src/main/java/com/pulseloop/widgets/WidgetSnapshotPublisher.kt +++ b/app/src/main/java/com/pulseloop/widgets/WidgetSnapshotPublisher.kt @@ -105,7 +105,8 @@ object WidgetSnapshotPublisher { try { val db = PulseLoopDatabase.getInstance(context) val keyStore = ApiKeyStore(context) - val snapshot = buildSnapshot(db, keyStore) + val preferPhone = com.pulseloop.settings.StepSourcePrefs(context).preferPhone() + val snapshot = buildSnapshot(db, keyStore, preferPhone) // Content comparison with a fixed timestamp (iOS `.distantPast` trick). val hash = WidgetSnapshotCodec.encode(snapshot.copy(generatedAt = 0)).hashCode() @@ -152,7 +153,11 @@ object WidgetSnapshotPublisher { * activity tile's unit conversion/formatting — so a widget tile is pixel- and label-identical * to its Today tile at publish time. */ - suspend fun buildSnapshot(db: PulseLoopDatabase, keyStore: ApiKeyStore?): WidgetSnapshot { + suspend fun buildSnapshot( + db: PulseLoopDatabase, + keyStore: ApiKeyStore?, + preferPhone: Boolean = false, + ): WidgetSnapshot { val nowMs = System.currentTimeMillis() val units = keyStore?.resolvedUnitSystem ?: UnitSystem.METRIC val vitals = VitalsViewModel.buildState(db, keyStore) @@ -180,18 +185,24 @@ object WidgetSnapshotPublisher { return WidgetSnapshot( generatedAt = nowMs / 1000L, dayStart = TimeUtil.startOfTodayLocal() / 1000L, - activity = activityPayload(db, units), + activity = activityPayload(db, units, preferPhone), sleep = sleepPayload(db), metrics = metrics, ) } /** Replicates the Today activity tile's value derivations (unit conversion, formatting). */ - private suspend fun activityPayload(db: PulseLoopDatabase, units: UnitSystem): WidgetActivityPayload { + private suspend fun activityPayload( + db: PulseLoopDatabase, + units: UnitSystem, + preferPhone: Boolean, + ): WidgetActivityPayload { val today = try { db.activityDailyDao().byDay(TimeUtil.startOfTodayLocal()) } catch (_: Exception) { null } val goal = try { db.userGoalDao().get() } catch (_: Exception) { null } - val steps = today?.steps + // Read-time column pick (Path B, PR #98): the ring's `steps` unless the user has chosen + // the phone AND the phone has a value for the day. Matches the app's Today tile. + val steps = today?.displaySteps(preferPhone) val calories = today?.calories val distanceDisplay = today?.distanceMeters?.let { UnitConverter.distance(it, units) } From 21f81c0a919097290da18135967a43da2cc4df87 Mon Sep 17 00:00:00 2001 From: leona Date: Tue, 6 Oct 2026 22:54:32 +0200 Subject: [PATCH 07/33] Delete the in-memory phone-ownership tracking --- .../service/EventPersistenceSubscriber.kt | 103 +++++------------- 1 file changed, 26 insertions(+), 77 deletions(-) diff --git a/app/src/main/java/com/pulseloop/service/EventPersistenceSubscriber.kt b/app/src/main/java/com/pulseloop/service/EventPersistenceSubscriber.kt index a12a14d8..79622435 100644 --- a/app/src/main/java/com/pulseloop/service/EventPersistenceSubscriber.kt +++ b/app/src/main/java/com/pulseloop/service/EventPersistenceSubscriber.kt @@ -46,40 +46,6 @@ class EventPersistenceSubscriber( private val scope = CoroutineScope(Dispatchers.IO + SupervisorJob()) private var job: Job? = null - /** The user's step-source preference. Plain SharedPreferences under the hood — cheap - * enough to read directly on every activity event, so no in-memory cache is needed. */ - private val stepSourcePrefs = StepSourcePrefs(context) - - /** - * Millis of the last successful [PulseEvent.PhoneStepsUpdate] this subscriber saw — i.e. - * the last time the phone reader actually managed to publish a value. 0 until the first - * successful read after process start, and reset to 0 on - * [PulseEvent.PhoneStepSourceUnavailable]. - */ - private var lastPhoneStepAt: Long = 0L - - /** - * True when the phone source should win over the ring. Requires **both** the user's - * preference and recent evidence the phone reader is working — a successful - * [PulseEvent.PhoneStepsUpdate] within [PHONE_STEP_GRACE_MS]. - * - * Without the freshness check, a revoked READ_STEPS, a removed HC provider, or a device - * with no step writer would leave the preference stuck on "phone" while - * [com.pulseloop.PhoneStepManager.refresh] silently failed, dropping the ring's live - * totals and freezing today's count with no path back. The grace period covers a long - * gap between app opens while still catching a real failure within a day. - * - * Read on every ring activity event, so the check is deliberately non-suspend and - * allocation-free — a plain timestamp comparison. - */ - private val stepSourceIsPhone: Boolean - get() { - if (stepSourcePrefs.stepSource != StepSourcePrefs.SOURCE_PHONE) return false - val last = lastPhoneStepAt - if (last == 0L) return false - return System.currentTimeMillis() - last < PHONE_STEP_GRACE_MS - } - // Battery-history throttle (iOS #61b) — in-memory, so the first reading after each (re)launch // always records; a change or a 30-min floor logs a fresh row otherwise, keeping the table to a // few dozen rows/day instead of one per BLE battery read. @@ -417,11 +383,10 @@ class EventPersistenceSubscriber( else db.measurementDao().insert(measurement) } is PulseEvent.ActivityUpdate -> { - // When the user picks the phone as their step source, ignore the ring's - // live activity totals so its over-counted steps don't land in the DB. - if (!stepSourceIsPhone) { - upsertActivityDaily(event.timestamp.toEpochMilli(), event.steps, event.calories, event.distanceMeters) - } + // The ring always writes its own totals, in every mode. Under Path B the phone's + // count lives in `phoneSteps`, so there is no ownership conflict to gate on — the + // read path picks which column to show at display time. + upsertActivityDaily(event.timestamp.toEpochMilli(), event.steps, event.calories, event.distanceMeters) } is PulseEvent.ActivityBucket -> { // Per-slice ring history: upserted by timestamp + the day total recomputed as the @@ -431,17 +396,12 @@ class EventPersistenceSubscriber( applyActivityBucket(event.timestamp.toEpochMilli(), event.steps, event.distanceMeters) } is PulseEvent.PhoneStepsUpdate -> { - // Fresh phone value — refresh the persisted health timestamp that gates the - // ring fallback (see [stepSourceIsPhone]). - lastPhoneStepAt = System.currentTimeMillis() upsertActivityDailyFromPhone(event.timestamp.toEpochMilli(), event.steps) } - is PulseEvent.PhoneStepSourceUnavailable -> { - // A5: the phone reader just failed. Reset the health marker so [stepSourceIsPhone] - // stops gating the ring's live totals off; the ring fills in until the - // phone reader recovers on a later foreground. - lastPhoneStepAt = 0L - } + // The phone reader just failed. Under Path B, nothing to do here: the ring writes its + // own column regardless, and the read-time pick falls back to `steps` when a day has + // no `phoneSteps` value. The event still exists on the bus for the debug screen. + is PulseEvent.PhoneStepSourceUnavailable -> Unit is PulseEvent.SleepTimeline -> { upsertSleepSession(event.timestamp.toEpochMilli(), event.stages, event.completeSession) } @@ -548,10 +508,13 @@ class EventPersistenceSubscriber( val deletion = com.pulseloop.data.ActivityBucketDeletion val hasDeletion = existing.deletedSteps > 0 || existing.deletedDistanceMeters > 0.0 db.activityDailyDao().upsert(existing.copy( - // A4: if the row was previously marked "phone" and the ring is now writing - // (this path only runs when [stepSourceIsPhone] is false), reset the marker so - // the row's source reflects its current owner. Other sources - // ("ring_history", "manual_recording", "hr_and_manual") are preserved. + // A row the phone created first (`source == "phone"`) flips to `"ring"` when the + // ring writes — the `source` column still has two consumers downstream: the + // Health Connect exporter (via `HealthConnectTypeMappings.EXCLUDED_SOURCES`) and + // [DailyCalorieEstimator.deviceReportedCalories]. Both read it as "who last wrote + // these columns", so once the ring has supplied `steps`/`calories`/`distance`, + // the row belongs to the ring. Other sources ("ring_history", "manual_recording", + // "hr_and_manual") are preserved as before. source = if (existing.source == StepSourcePrefs.SOURCE_PHONE) StepSourcePrefs.SOURCE_RING else existing.source, steps = if (stale) steps else deletion.ratchetAgainstRing(existing.steps, steps, existing.deletedSteps), @@ -661,25 +624,18 @@ class EventPersistenceSubscriber( val isToday = dayStart == com.pulseloop.util.TimeUtil.startOfTodayLocal() val stale = existing != null && existing.steps > 200_000 val ratchet = isToday && existing != null && !stale - // A4: "owned" means the row was written by the phone AND the phone source is still - // currently winning (the same condition [stepSourceIsPhone] checks — user preference - // plus recent successful reads). If the user has switched back to Ring, or the phone - // reader has gone stale, the ring's bucket sum must be allowed to retake the day; - // otherwise a single toggle to Phone would suppress the ring's totals for those days - // permanently. - val dayOwnedByPhone = existing?.source == StepSourcePrefs.SOURCE_PHONE && stepSourceIsPhone - val skipDailyTotal = dayOwnedByPhone || (stepSourceIsPhone && isToday) - if (!skipDailyTotal) { - db.activityDailyDao().upsert( - (existing ?: ActivityDailyEntity(date = dayStart, source = "ring_history")).copy( - steps = if (ratchet) maxOf(existing!!.steps, totalSteps) else totalSteps, - distanceMeters = if (ratchet) maxOf(existing!!.distanceMeters, totalDistance) else totalDistance, - source = if (ratchet) existing!!.source else "ring_history", - syncedAt = System.currentTimeMillis(), - updatedAt = System.currentTimeMillis(), - ) + // Under Path B the ring always writes: the phone never occupies `steps`, so there is no + // ownership gate to skip this upsert. The phone's value lives in `phoneSteps`, untouched + // by anything here. + db.activityDailyDao().upsert( + (existing ?: ActivityDailyEntity(date = dayStart, source = "ring_history")).copy( + steps = if (ratchet) maxOf(existing!!.steps, totalSteps) else totalSteps, + distanceMeters = if (ratchet) maxOf(existing!!.distanceMeters, totalDistance) else totalDistance, + source = if (ratchet) existing!!.source else "ring_history", + syncedAt = System.currentTimeMillis(), + updatedAt = System.currentTimeMillis(), ) - } + ) } private suspend fun upsertSleepSession(ts: Long, stages: List, completeSession: Boolean) { @@ -936,13 +892,6 @@ class EventPersistenceSubscriber( /** How far back [spotReadings] is primed from the table on first use. */ const val SPOT_LOOKBACK_MS = 7L * 24 * 60 * 60_000 private const val MAX_SLEEP_TIMELINE_MINUTES = 24 * 60 - - /** - * How long the subscriber trusts a "phone" preference without a successful phone read - * before falling back to the ring (see [stepSourceIsPhone]). Covers a long gap - * between app opens while still catching a real failure within a day. - */ - private const val PHONE_STEP_GRACE_MS = 12L * 3_600_000L } } From 98b606acafe04ba3957698936ae6011f186d249d Mon Sep 17 00:00:00 2001 From: leona Date: Tue, 6 Oct 2026 23:04:25 +0200 Subject: [PATCH 08/33] Exporter: drop the phone exclusion; plausibility guards already skip empty days --- .../health/HealthConnectTypeMappings.kt | 30 +++++++++++-------- .../health/HealthConnectTypeMappingsTest.kt | 18 +++++++++++ 2 files changed, 36 insertions(+), 12 deletions(-) diff --git a/app/src/main/java/com/pulseloop/health/HealthConnectTypeMappings.kt b/app/src/main/java/com/pulseloop/health/HealthConnectTypeMappings.kt index 5fb321d2..eb034129 100644 --- a/app/src/main/java/com/pulseloop/health/HealthConnectTypeMappings.kt +++ b/app/src/main/java/com/pulseloop/health/HealthConnectTypeMappings.kt @@ -4,7 +4,6 @@ import androidx.health.connect.client.records.ExerciseSessionRecord import androidx.health.connect.client.records.MealType import androidx.health.connect.client.records.SleepSessionRecord import com.pulseloop.ring.SleepStage -import com.pulseloop.settings.StepSourcePrefs import java.time.Instant import java.time.ZoneId import java.time.ZoneOffset @@ -26,18 +25,25 @@ import java.time.ZoneOffset object HealthConnectTypeMappings { /** - * `source` / `sourceRaw` values whose rows must never be exported to Health Connect. + * `source` / `sourceRaw` values whose rows must never be exported to Health Connect — + * seeded placeholder data (mirrors iOS, which never exports demo/mock). * - * - `demo`, `mock`: seeded placeholder data (mirrors iOS, which never exports demo/mock). - * - `phone`: a phone-sourced daily total is *Health Connect's own data* — the phone reader - * got it from Health Connect in the first place. Re-exporting it would create a - * self-feedback loop: PulseLoop writes the phone's step count back, then reads it back - * as if it were the phone's, then writes it again. [com.pulseloop.PhoneStepManager] also - * filters PulseLoop's own origin out at read time; this exclusion and that filter are - * the two halves of the same fix. See `ActivityExporter.build` for the filtering call - * site. + * `"phone"` used to be listed here as well, to break a self-feedback loop: back when the + * phone write path overwrote `activity_daily.steps` — the very column this exporter reads — + * PulseLoop would re-export Health Connect's own data back to Health Connect, and the phone + * reader would then read it back as if it were the phone's. Under the separate-column design + * (PR #98) the phone's step count lives in `phoneSteps`, which this exporter never reads, so + * the loop is broken by construction and the exclusion is gone. `PhoneStepManager`'s + * own-origin subtraction at read time is unaffected and still runs — it cleans up the + * historic exports that already exist in Health Connect. + * + * A phone-first day (`source == "phone"`, `steps == 0`, `calories == 0.0`, + * `distanceMeters == 0.0`) still produces no record, and does so on its own merits: every + * metric fails its plausibility guard (`isPlausibleSteps` floors at 1, the other two at + * strictly > 0), so the day drops through the same path as any other empty day. It no + * longer needs a source-based shortcut to be skipped. */ - val EXCLUDED_SOURCES = setOf("demo", "mock", StepSourcePrefs.SOURCE_PHONE) + val EXCLUDED_SOURCES = setOf("demo", "mock") const val HOUR_MS = 3_600_000L @@ -745,4 +751,4 @@ object HealthConnectTypeMappings { "snack" -> MealType.MEAL_TYPE_SNACK else -> MealType.MEAL_TYPE_UNKNOWN } -} \ No newline at end of file +} diff --git a/app/src/test/java/com/pulseloop/health/HealthConnectTypeMappingsTest.kt b/app/src/test/java/com/pulseloop/health/HealthConnectTypeMappingsTest.kt index e72f9af7..63578b28 100644 --- a/app/src/test/java/com/pulseloop/health/HealthConnectTypeMappingsTest.kt +++ b/app/src/test/java/com/pulseloop/health/HealthConnectTypeMappingsTest.kt @@ -366,4 +366,22 @@ class HealthConnectTypeMappingsTest { val out = HealthConnectTypeMappings.normalizeSleepStages(s0, s1, listOf(span(-120, -60), span(300, 360))) assertTrue(out.isEmpty()) } + + // ── EXCLUDED_SOURCES ── + + @Test + fun excludedSourcesCoversDemoAndMock() { + // Seed/placeholder rows must never leave the app (mirrors iOS). + // + // `"phone"` used to be in this set as well, to break a Health Connect self-feedback loop + // under the old design where the phone wrote its step count into `activity_daily.steps` + // — the same column the exporter reads. Under the separate-column design (PR #98) the + // phone's value lives in `phoneSteps`, which the exporter never reads, so the loop is + // broken by construction and the exclusion is gone. A phone-first day still produces no + // record — but via the plausibility guards (`isPlausibleSteps` floors at 1) rather than a + // source-based skip. See the KDoc on HealthConnectTypeMappings.EXCLUDED_SOURCES. + assertTrue(HealthConnectTypeMappings.EXCLUDED_SOURCES.contains("demo")) + assertTrue(HealthConnectTypeMappings.EXCLUDED_SOURCES.contains("mock")) + assertFalse(HealthConnectTypeMappings.EXCLUDED_SOURCES.contains("phone")) + } } From 3b50a78edd0bdcab8a2480c1a0f8872317a1a506 Mon Sep 17 00:00:00 2001 From: leona Date: Wed, 7 Oct 2026 21:52:58 +0200 Subject: [PATCH 09/33] Fix Step Source switch: permission filter + rapid-toggle race --- .../pulseloop/ui/screens/SettingsScreen.kt | 37 +++++++++++++++---- 1 file changed, 30 insertions(+), 7 deletions(-) diff --git a/app/src/main/java/com/pulseloop/ui/screens/SettingsScreen.kt b/app/src/main/java/com/pulseloop/ui/screens/SettingsScreen.kt index 0d6bdb20..a054cc42 100644 --- a/app/src/main/java/com/pulseloop/ui/screens/SettingsScreen.kt +++ b/app/src/main/java/com/pulseloop/ui/screens/SettingsScreen.kt @@ -26,9 +26,20 @@ import com.pulseloop.ui.components.SettingsRowItem import com.pulseloop.ui.components.SettingsSection import com.pulseloop.ui.theme.PulseColors import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.Job import kotlinx.coroutines.launch import kotlinx.coroutines.withContext +/** + * READ_STEPS permission string, resolved once. Referenced by both the Step Source row's + * "switch to Phone" path (to test whether the permission is already granted before + * asking) and the permission-result launcher's callback (to verify the user actually + * granted *this* permission, not some other one). + */ +private val READ_STEPS_PERMISSION: String = + androidx.health.connect.client.permission.HealthPermission + .getReadPermission(androidx.health.connect.client.records.StepsRecord::class) + /** * Ported from SettingsView.swift (iOS #49 rehaul). * Top-level Settings: a hero ring-device card over grouped sections of navigation rows. Each @@ -63,6 +74,11 @@ fun SettingsScreen( val developerUnlocked = keyStore.developerUnlocked var stepSource by remember { mutableStateOf(stepSourcePrefs.stepSource) } + // In-flight "switch to Phone" attempt. Cancelled before each new attempt so a + // rapid Ring → Phone → Ring toggle cannot have an older attempt's async HC + // checks land after the user's latest intent and re-persist "phone". + var switchJob by remember { mutableStateOf(null) } + // Provider-aware AI Coach summary — mirrors iOS `coachTrailing` (no Apple on-device // mode on Android; hosted providers show the selected model slug). val coachTrailing = if (!coachEnabled) "Off" else when (providerStore.providerMode) { @@ -87,7 +103,7 @@ fun SettingsScreen( val healthConnectPermissionLauncher = rememberLauncherForActivityResult( androidx.health.connect.client.PermissionController.createRequestPermissionResultContract() ) { granted -> - if (granted.isNotEmpty()) { + if (READ_STEPS_PERMISSION in granted) { // Granted — now it is safe to persist "phone" and run the backfill. A6: the // preference is written only after permission is confirmed, so an interrupted // flow cannot leave the app claiming a source it can't read from. @@ -199,6 +215,10 @@ fun SettingsScreen( ) { val newSource = if (stepSource == StepSourcePrefs.SOURCE_RING) StepSourcePrefs.SOURCE_PHONE else StepSourcePrefs.SOURCE_RING stepSource = newSource // optimistic UI; persisted only once proven valid + // Cancel any previous in-flight attempt: the user's latest tap is the + // only intent that matters, and a stale attempt must not persist + // "phone" after the user has since flipped back to Ring. + switchJob?.cancel() if (newSource == StepSourcePrefs.SOURCE_RING) { // Ring is always safe — no external dependency to verify. stepSourcePrefs.stepSource = StepSourcePrefs.SOURCE_RING @@ -207,7 +227,7 @@ fun SettingsScreen( // BEFORE persisting. The old flow saved "phone" first and reverted // on failure, so an interrupted run left the app claiming a source // it couldn't read from, which then dropped the ring's live totals. - scope.launch { + switchJob = scope.launch { val availability = withContext(Dispatchers.IO) { HealthConnectSdk.availability(context) } @@ -225,18 +245,21 @@ fun SettingsScreen( stepSource = StepSourcePrefs.SOURCE_RING // revert the optimistic flip return@launch } - val readSteps = androidx.health.connect.client.permission.HealthPermission - .getReadPermission(androidx.health.connect.client.records.StepsRecord::class) val hasPermission = withContext(Dispatchers.IO) { try { val client = androidx.health.connect.client.HealthConnectClient .getOrCreate(context) - readSteps in client.permissionController.getGrantedPermissions() + READ_STEPS_PERMISSION in client.permissionController.getGrantedPermissions() } catch (_: Exception) { false } } if (hasPermission) { + // Re-check intent immediately before persisting: the user + // may have flipped back to Ring while the permission query + // was in flight, and cancellation is cooperative — this + // coroutine can reach here with the cancel flag set. + if (stepSource != StepSourcePrefs.SOURCE_PHONE) return@launch // Already granted — persist and backfill now. stepSourcePrefs.stepSource = StepSourcePrefs.SOURCE_PHONE withContext(Dispatchers.IO) { @@ -250,7 +273,7 @@ fun SettingsScreen( } else { // Not granted — ask. The launcher's callback persists // "phone" only once permission is confirmed. - healthConnectPermissionLauncher.launch(setOf(readSteps)) + healthConnectPermissionLauncher.launch(setOf(READ_STEPS_PERMISSION)) } } } @@ -292,4 +315,4 @@ fun SettingsScreen( Spacer(Modifier.height(32.dp)) } -} \ No newline at end of file +} From ce32b213d8bd2025bfd63c7f69654fc42c63e505 Mon Sep 17 00:00:00 2001 From: leona Date: Wed, 7 Oct 2026 21:59:36 +0200 Subject: [PATCH 10/33] PulseEventBus: hold events until first subscriber attaches --- app/src/main/java/com/pulseloop/ring/PulseEventBus.kt | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/app/src/main/java/com/pulseloop/ring/PulseEventBus.kt b/app/src/main/java/com/pulseloop/ring/PulseEventBus.kt index a00a205c..dd2b93b3 100644 --- a/app/src/main/java/com/pulseloop/ring/PulseEventBus.kt +++ b/app/src/main/java/com/pulseloop/ring/PulseEventBus.kt @@ -8,6 +8,7 @@ import kotlinx.coroutines.channels.Channel import kotlinx.coroutines.flow.MutableSharedFlow import kotlinx.coroutines.flow.SharedFlow import kotlinx.coroutines.flow.asSharedFlow +import kotlinx.coroutines.flow.first import kotlinx.coroutines.launch /** @@ -184,6 +185,14 @@ object PulseEventBus { init { dispatchScope.launch { + // Hold events until at least one subscriber is attached. On cold start, + // MainActivity.onResume runs refreshPhoneSteps() before PulseLoopApp's + // LaunchedEffect has called persistence.start() — the composition hasn't run + // yet. With replay = 0, events published in that window are emitted to zero + // subscribers and lost, including today's PhoneStepsUpdate and every backfill + // event. Waiting here defers the drain until the subscriber exists; once it + // does, the whole backlog is delivered in order. + _events.subscriptionCount.first { it > 0 } // The bus is process-long and single-drained: an uncaught throw here would kill the // dispatcher and silently stop every subscriber for the rest of the process (the // SupervisorJob does not restart it). Isolate each emit so one bad event can't do that. @@ -210,4 +219,4 @@ object PulseEventBus { android.util.Log.e("PulseEventBus", "Pulse event queue rejected ${event.javaClass.simpleName}", result.exceptionOrNull()) } } -} +} \ No newline at end of file From 7bf8921f6a5fef8e44a4fd65a6ef5b73c57341f9 Mon Sep 17 00:00:00 2001 From: leona Date: Wed, 7 Oct 2026 22:06:02 +0200 Subject: [PATCH 11/33] PhoneStepManager: filter foreign origins instead of subtracting our own --- .../java/com/pulseloop/PhoneStepManager.kt | 159 +++++++++--------- 1 file changed, 79 insertions(+), 80 deletions(-) diff --git a/app/src/main/java/com/pulseloop/PhoneStepManager.kt b/app/src/main/java/com/pulseloop/PhoneStepManager.kt index 75ff51b0..1431d18c 100644 --- a/app/src/main/java/com/pulseloop/PhoneStepManager.kt +++ b/app/src/main/java/com/pulseloop/PhoneStepManager.kt @@ -7,6 +7,7 @@ import androidx.health.connect.client.records.StepsRecord import androidx.health.connect.client.records.metadata.DataOrigin import androidx.health.connect.client.request.AggregateGroupByPeriodRequest import androidx.health.connect.client.request.AggregateRequest +import androidx.health.connect.client.request.ReadRecordsRequest import androidx.health.connect.client.time.TimeRangeFilter import com.pulseloop.ring.PulseEvent import com.pulseloop.ring.PulseEventBus @@ -36,21 +37,24 @@ import java.time.ZoneId * distance from [com.pulseloop.service.ActivityRollup]. Baking either into the event * would overwrite those paths on write. * - * **Every read subtracts PulseLoop's own origin.** PulseLoop exports its own daily step - * totals to Health Connect (`ActivityExporter`), so a naive aggregate would include our - * own earlier exports — the ring's daily total fed back to the phone path, or the - * phone's own total plus the ring's. Both reads therefore run twice: once with no origin - * filter (all origins), once filtered to PulseLoop's own package, and the second is - * subtracted from the first. `ActivityExporter` also skips `source == "phone"` rows, - * which stops the loop from growing; this read-side subtraction cleans up the exports - * that already exist. + * **Every read excludes PulseLoop's own origin.** PulseLoop exports its own daily step + * totals to Health Connect (`ActivityExporter`), so an aggregate that included our own + * origin would feed the ring's (or a previous phone-mode run's) exported value back + * into the phone path — the exported total plus the real one, or the exported total + * published as "phone steps" on a day the phone has no source. Health Connect's + * `aggregate` de-duplicates overlapping records by priority rather than summing, so a + * manual subtraction of our own aggregate (the previous approach) can't work: the + * unfiltered aggregate picks our own export as the priority winner, and subtracting it + * then zeroes out the real phone total on any day PulseLoop has exported. Instead we + * **read the record origins, drop our own package, and pass the remaining origins to + * `aggregate`'s `dataOriginFilter`** — HC's de-duplication then runs over foreign + * sources only, which is the intended pool. */ class PhoneStepManager(private val context: Context) { /** - * PulseLoop's own origin, as Health Connect identifies it. Passed as the - * `dataOriginFilter` for the "own contribution" aggregate that gets subtracted from - * the unfiltered one — see the class KDoc. + * PulseLoop's own origin, as Health Connect identifies it. Excluded from the origins + * discovered for a read — see the class KDoc. */ private val ownOrigin: Set = setOf(DataOrigin(context.packageName)) @@ -77,24 +81,36 @@ class PhoneStepManager(private val context: Context) { val now = Instant.now() val startOfDayMs = TimeUtil.startOfDayLocal(System.currentTimeMillis()) val startOfDay = Instant.ofEpochMilli(startOfDayMs) + val range = TimeRangeFilter.between(startOfDay, now) - // Two aggregates and a subtraction: all origins minus PulseLoop's own contribution. - // See the class KDoc for why. The result is floored at 0 because a momentarily - // inconsistent pairing of the two calls could otherwise produce a negative step - // count — not a value we would ever want to publish. - val allSteps = aggregateToday(client, startOfDay, now, dataOriginFilter = emptySet()) - ?: return false - val ownSteps = aggregateToday(client, startOfDay, now, dataOriginFilter = ownOrigin) - ?: return false - val steps = (allSteps - ownSteps).coerceAtLeast(0) - - // Just after midnight, or on a phone with no step writer in Health Connect, the - // aggregate returns null and becomes 0. Publishing 0 would clobber the ring's - // only valid record for the day and mark the row source = "phone", which then - // also blocks the ring's bucket-derived total for today. Skip the publish and - // let the ring's number stand until the phone has actually counted some steps. - // The query itself succeeded, so still return true. + // Discover foreign origins writing steps today; aggregate only over them. See the + // class KDoc for why HC's own subtraction doesn't work. + val foreignOrigins = try { + foreignStepOrigins(client, range) + } catch (_: Exception) { + return false + } + if (foreignOrigins.isEmpty()) { + // No phone step source has records for today. Skip the publish so the ring's + // value for today stands; the query itself succeeded, so return true. + return true + } + + val steps = try { + client.aggregate( + AggregateRequest( + metrics = setOf(StepsRecord.COUNT_TOTAL), + timeRangeFilter = range, + dataOriginFilter = foreignOrigins, + ) + )[StepsRecord.COUNT_TOTAL]?.toInt() ?: 0 + } catch (_: Exception) { + return false + } + // An aggregate of 0 with a non-empty origin set is possible but rare (origins have + // records, all of which total 0). Skip the publish for the same reason as above. if (steps <= 0) return true + PulseEventBus.publishBlocking( PulseEvent.PhoneStepsUpdate( timestamp = now, @@ -104,27 +120,6 @@ class PhoneStepManager(private val context: Context) { return true } - /** - * One aggregate over today's `StepsRecord`s, filtered to [dataOriginFilter] when - * non-empty. Returns null on any client error so the caller can short-circuit. - */ - private suspend fun aggregateToday( - client: HealthConnectClient, - startOfDay: Instant, - now: Instant, - dataOriginFilter: Set, - ): Int? = try { - client.aggregate( - AggregateRequest( - metrics = setOf(StepsRecord.COUNT_TOTAL), - timeRangeFilter = TimeRangeFilter.between(startOfDay, now), - dataOriginFilter = dataOriginFilter, - ) - )[StepsRecord.COUNT_TOTAL]?.toInt() ?: 0 - } catch (_: Exception) { - null - } - /** * Query Health Connect for step totals per day across a rolling window, and publish each * one as a separate [PulseEvent.PhoneStepsUpdate]. Used on app start / toggle change to @@ -150,11 +145,8 @@ class PhoneStepManager(private val context: Context) { * today's data. We anchor the range to local midnight so every bucket is exactly one * calendar day. * - * 3. Every emitted total is the **difference** between the unfiltered aggregate and the - * PulseLoop-only aggregate — see the class KDoc. The two calls share the range and - * slicer, so their buckets line up by `startTime`; a bucket present only in the - * unfiltered result is a day where PulseLoop has no records, so the subtraction is - * just the unfiltered value. + * 3. Origins are discovered from the raw records first, then the aggregate runs with + * `dataOriginFilter` set to the foreign origins only — see the class KDoc for why. * * @param daysBack how many days of history to backfill (default 30). * @return true if the query succeeded (even if some days were empty), false on failure. @@ -181,47 +173,36 @@ class PhoneStepManager(private val context: Context) { val todayStart: LocalDateTime = LocalDate.now().atStartOfDay() val endLocal: LocalDateTime = todayStart.plusDays(1) val startLocal: LocalDateTime = todayStart.minusDays(daysBack - 1) + val range = TimeRangeFilter.between(startLocal, endLocal) - // Two aggregateGroupByPeriod calls, same range and slicer, differing only in the - // origin filter. See the class KDoc. - val allBuckets = try { - client.aggregateGroupByPeriod( - AggregateGroupByPeriodRequest( - metrics = setOf(StepsRecord.COUNT_TOTAL), - timeRangeFilter = TimeRangeFilter.between(startLocal, endLocal), - timeRangeSlicer = Period.ofDays(1), - dataOriginFilter = emptySet(), - ) - ) + // Discover foreign origins across the whole window; aggregate only over them. See + // the class KDoc for why HC's own subtraction doesn't work. + val foreignOrigins = try { + foreignStepOrigins(client, range) } catch (_: Exception) { return false } - val ownBuckets = try { + if (foreignOrigins.isEmpty()) return true + + val buckets = try { client.aggregateGroupByPeriod( AggregateGroupByPeriodRequest( metrics = setOf(StepsRecord.COUNT_TOTAL), - timeRangeFilter = TimeRangeFilter.between(startLocal, endLocal), + timeRangeFilter = range, timeRangeSlicer = Period.ofDays(1), - dataOriginFilter = ownOrigin, + dataOriginFilter = foreignOrigins, ) ) } catch (_: Exception) { return false } - // Index PulseLoop's own per-day totals by bucket start time so the subtraction below - // is a single map lookup per bucket. A bucket only in `allBuckets` is a day where - // PulseLoop has no records — the subtraction is just `all - 0`. - val ownByStart: Map = ownBuckets.associate { bucket -> - bucket.startTime to (bucket.result[StepsRecord.COUNT_TOTAL]?.toInt() ?: 0) - } - // Each bucket is one day's total. The bucket's startTime marks the beginning of the - // period it covers (local midnight). Map each non-empty bucket to a PhoneStepsUpdate - // so the persistence layer overwrites the corresponding activity_daily row. - for (bucket in allBuckets) { - val all = bucket.result[StepsRecord.COUNT_TOTAL]?.toInt() ?: 0 - val own = ownByStart[bucket.startTime] ?: 0 - val steps = (all - own).coerceAtLeast(0) + // Each bucket is one day's total over the foreign origins. The bucket's startTime + // marks the beginning of the period it covers (local midnight). Map each non-empty + // bucket to a PhoneStepsUpdate so the persistence layer overwrites the corresponding + // activity_daily row. + for (bucket in buckets) { + val steps = bucket.result[StepsRecord.COUNT_TOTAL]?.toInt() ?: 0 // Skip empty days: publishing 0 would clobber the ring's only valid record for // that day. Leaving the day untouched lets it fall back to the ring's data. if (steps <= 0) continue @@ -236,4 +217,22 @@ class PhoneStepManager(private val context: Context) { } return true } + + /** + * The set of Health Connect origins writing `StepsRecord`s in [timeRangeFilter], minus + * PulseLoop's own. The result is passed as `dataOriginFilter` to the aggregates above so + * HC's priority-based de-duplication runs over foreign sources only — see the class KDoc. + */ + private suspend fun foreignStepOrigins( + client: HealthConnectClient, + timeRangeFilter: TimeRangeFilter, + ): Set { + val records = client.readRecords( + ReadRecordsRequest( + recordType = StepsRecord::class, + timeRangeFilter = timeRangeFilter, + ) + ).records + return records.map { it.metadata.dataOrigin }.toSet() - ownOrigin + } } \ No newline at end of file From f13752ad2b9a01bb30a80c282627896d5505b71f Mon Sep 17 00:00:00 2001 From: leona Date: Wed, 7 Oct 2026 22:09:41 +0200 Subject: [PATCH 12/33] PhoneStepManager: publish 0 for today so phone mode reads 0, not stale ring --- .../java/com/pulseloop/PhoneStepManager.kt | 56 +++++++++++-------- 1 file changed, 34 insertions(+), 22 deletions(-) diff --git a/app/src/main/java/com/pulseloop/PhoneStepManager.kt b/app/src/main/java/com/pulseloop/PhoneStepManager.kt index 1431d18c..d2a4a627 100644 --- a/app/src/main/java/com/pulseloop/PhoneStepManager.kt +++ b/app/src/main/java/com/pulseloop/PhoneStepManager.kt @@ -62,6 +62,17 @@ class PhoneStepManager(private val context: Context) { * Query Health Connect for today's step total and publish it as a * [PulseEvent.PhoneStepsUpdate]. Returns true when the query succeeded, false when * Health Connect is unavailable or the read permission has not been granted yet. + * + * **Today always publishes, including 0.** A day's phone total of 0 is a real state — + * just after midnight, before the phone has counted anything, the correct phone-mode + * display is 0, not yesterday's ring total still sitting in the `steps` column. Under + * the separate-column design ([ActivityDailyEntity.phoneSteps]), publishing 0 to + * `phoneSteps` is how "today's phone value is 0" becomes visible: `displaySteps` picks + * `phoneSteps` when it is non-null and falls back to the ring only when the phone has + * never written for the day. Skipping the 0 would leave today's `phoneSteps` null and + * the display showing the ring's stale value — the exact midnight gap the previous + * ownership-tracking design produced. `upsertActivityDailyFromPhone` is a no-op when + * the value is unchanged, so re-publishing the same 0 on every foreground is free. */ suspend fun refresh(): Boolean { val client = try { @@ -90,26 +101,26 @@ class PhoneStepManager(private val context: Context) { } catch (_: Exception) { return false } - if (foreignOrigins.isEmpty()) { - // No phone step source has records for today. Skip the publish so the ring's - // value for today stands; the query itself succeeded, so return true. - return true - } - val steps = try { - client.aggregate( - AggregateRequest( - metrics = setOf(StepsRecord.COUNT_TOTAL), - timeRangeFilter = range, - dataOriginFilter = foreignOrigins, - ) - )[StepsRecord.COUNT_TOTAL]?.toInt() ?: 0 - } catch (_: Exception) { - return false + val steps = if (foreignOrigins.isEmpty()) { + // No foreign origin has written steps today yet — a normal state just after + // midnight, and also the permanent state on a phone with no step writer. Either + // way today's phone total is 0. `aggregate` with an empty `dataOriginFilter` + // means "all origins", not "none", so it must not be called here. + 0 + } else { + try { + client.aggregate( + AggregateRequest( + metrics = setOf(StepsRecord.COUNT_TOTAL), + timeRangeFilter = range, + dataOriginFilter = foreignOrigins, + ) + )[StepsRecord.COUNT_TOTAL]?.toInt() ?: 0 + } catch (_: Exception) { + return false + } } - // An aggregate of 0 with a non-empty origin set is possible but rare (origins have - // records, all of which total 0). Skip the publish for the same reason as above. - if (steps <= 0) return true PulseEventBus.publishBlocking( PulseEvent.PhoneStepsUpdate( @@ -127,9 +138,10 @@ class PhoneStepManager(private val context: Context) { * show the phone's numbers rather than whatever the ring recorded. * * Days where Health Connect has no phone-step records are skipped — those days fall back - * to the ring's data, which is the only source available for them. A day where the phone - * walked zero steps would publish `0` and clobber the ring's only valid record for that - * day, so the `steps <= 0` guard skips those buckets entirely. + * to the ring's data, which is the only source available for them. Unlike today's read + * ([refresh], which always publishes including 0), a past day with no phone records is + * genuinely unknown: the phone may not have been tracking then, so a 0 would wrongly hide + * a ring day that does have data. Only today's 0 is meaningful. * * **Three Health Connect quirks handled here:** * @@ -235,4 +247,4 @@ class PhoneStepManager(private val context: Context) { ).records return records.map { it.metadata.dataOrigin }.toSet() - ownOrigin } -} \ No newline at end of file +} From 7a187648e0e48fb9844e491dc71954ae6ec18dcb Mon Sep 17 00:00:00 2001 From: leona Date: Wed, 7 Oct 2026 22:22:04 +0200 Subject: [PATCH 13/33] Activity: calorie goal ring uses active, not total, calories UserGoal.calories is an active-energy goal, but the ring was drawn against the day's total calories (basal + active), so it sat near full from midnight. - ActivityState gains effectiveActiveCalories; the shown-day collector fills it from DailyCalorieEstimator.effectiveActiveCalories. - DailyActivitySummaryCard: the Calories metric still shows the total (matches the Today card); the third ActivityRing reads the active figure. - Records-card gating comment reworded for Path B (no ownership concept; the ring-bucket list still doesn't sum to the phone-sourced total, so the card stays hidden in Phone mode). --- .../pulseloop/ui/screens/ActivityScreen.kt | 21 ++++++++++++------- .../com/pulseloop/ui/viewmodels/ViewModels.kt | 17 +++++++++++++++ 2 files changed, 30 insertions(+), 8 deletions(-) diff --git a/app/src/main/java/com/pulseloop/ui/screens/ActivityScreen.kt b/app/src/main/java/com/pulseloop/ui/screens/ActivityScreen.kt index e5dee641..eb03de91 100644 --- a/app/src/main/java/com/pulseloop/ui/screens/ActivityScreen.kt +++ b/app/src/main/java/com/pulseloop/ui/screens/ActivityScreen.kt @@ -237,10 +237,10 @@ fun ActivityScreen( // after reading the number. // // Hidden when the step source is "Phone": the Records list is the ring's intraday - // bucket log, and while the phone owns the daily total, showing ring-derived blocks - // beneath that total reads as a bug — the sum of the records does not match the header - // above them, and deleting one has no effect on a total the ring no longer owns. - // Toggling back to "Ring" restores the card. + // bucket log, and under Path B the header above it is the phone's total (chosen at + // read time from `phoneSteps`), so the records no longer sum to the number they sit + // under — reading as a bug rather than a detail view. Deleting a ring bucket would + // also not move the phone-sourced total. Toggling back to "Ring" restores the card. if (stepSourcePrefs.stepSource != com.pulseloop.settings.StepSourcePrefs.SOURCE_PHONE) { item { ActivityRecordsCard( @@ -381,7 +381,12 @@ private fun DailyActivitySummaryCard(state: ActivityViewModel.ActivityState, uni // here would surface a phone row's stale hardcoded placeholder (or a ring's pre-toggle // value) as if it were a real total. Same read path as TodayViewModel's, so the two // screens cannot disagree. + // + // `calories` is the day's *total* (basal + active) — what the Calories metric shows, same + // as Today. The goal ring reads the active-only figure instead: UserGoal.calories is an + // active-energy goal, so a ring drawn against the total would sit near full from midnight. val calories = state.effectiveCalories + val activeCalories = state.effectiveActiveCalories Row( Modifier .fillMaxWidth() @@ -405,7 +410,7 @@ private fun DailyActivitySummaryCard(state: ActivityViewModel.ActivityState, uni rings = listOf( ActivityRing(today?.steps?.toDouble(), state.stepGoal.toDouble(), PulseColors.steps), ActivityRing(today?.distanceMeters, state.distanceGoalMeters, PulseColors.distance), - ActivityRing(calories, state.caloriesGoal.toDouble(), PulseColors.calories), + ActivityRing(activeCalories, state.caloriesGoal.toDouble(), PulseColors.calories), ), size = 112.dp, strokeWidth = 11.dp, @@ -888,8 +893,8 @@ private fun ActivityRecordsCard( text = { Text( "${Formats.count(bucket.steps)} steps at ${timeOf(bucket.startEpoch)}." + - "\n\nThe day's total drops by that much. This can't be undone, and the record " + - "stays deleted the next time this day syncs.", + "\n\nThe day's total drops by that much. This can't be undone, and the record " + + "stays deleted the next time this day syncs.", ) }, confirmButton = { @@ -905,4 +910,4 @@ private fun ActivityRecordsCard( } /** How many activity records the list shows before asking (issue #70). */ -private const val ACTIVITY_RECORDS_PAGE_SIZE = 50 +private const val ACTIVITY_RECORDS_PAGE_SIZE = 50 \ No newline at end of file diff --git a/app/src/main/java/com/pulseloop/ui/viewmodels/ViewModels.kt b/app/src/main/java/com/pulseloop/ui/viewmodels/ViewModels.kt index 980b99b3..331a96b8 100644 --- a/app/src/main/java/com/pulseloop/ui/viewmodels/ViewModels.kt +++ b/app/src/main/java/com/pulseloop/ui/viewmodels/ViewModels.kt @@ -470,8 +470,20 @@ class ActivityViewModel( * `phone`-sourced row would otherwise show its raw `calories` column, which is either * the estimator's own fill-in or the stale placeholder written before that path * existed. + * + * This is the *total* figure (basal + active), which is what the Calories metric text + * shows. The calorie goal ring reads [effectiveActiveCalories] instead — the stored + * goal is an active-energy goal ([UserGoalEntity.calories] KDoc), so a ring drawn + * against this total would sit near-full from the moment the day begins. */ val effectiveCalories: Double? = null, + /** + * The active-energy portion of the shown day's calories — the ring's own reported + * figure when it has one, else [DailyCalorieEstimator.effectiveActiveCalories]. + * Drives the calorie goal ring, whose goal ([UserGoalEntity.calories]) is in the same + * active-energy units. Null when the row has neither a device figure nor an estimate. + */ + val effectiveActiveCalories: Double? = null, val stepGoal: Int = UserGoalEntity.DEFAULT_STEPS, val activeMinutesGoal: Int = 45, val distanceGoalMeters: Double = UserGoalEntity.DEFAULT_DISTANCE_METERS, @@ -525,10 +537,15 @@ class ActivityViewModel( profile?.let { DailyCalorieEstimator.effectiveCalories(row, it) } ?: DailyCalorieEstimator.deviceReportedCalories(row) } + // The active-energy read: no profile needed — this is either the device's own + // figure or the stored estimate, both already net of BMR. See the state field's + // KDoc for why the ring uses this and the metric text does not. + val activeCalories = day?.let { DailyCalorieEstimator.effectiveActiveCalories(it) } _state.update { it.copy( daySummary = day?.forDisplay(stepSourcePrefs.preferPhone()), effectiveCalories = calories, + effectiveActiveCalories = activeCalories, ) } } From d7fa93df17a30869b314a5e3ad51872e56e85a72 Mon Sep 17 00:00:00 2001 From: leona Date: Wed, 7 Oct 2026 22:30:42 +0200 Subject: [PATCH 14/33] PhoneStepManager: share the HC client + steps-permission preamble The client construction and READ_STEPS check were copied across refresh(), refreshHistoricalDays(), and SettingsScreen's switch-to-Phone handler. Extracted to PhoneStepManager's companion so there is one source of truth for what 'granted and usable' means. Also hoists the READ_STEPS_PERMISSION constant to the same companion (SettingsScreen's file-private val is deleted) so the permission string cannot drift between the two call sites. --- .../java/com/pulseloop/PhoneStepManager.kt | 67 ++++++++++++------- .../pulseloop/ui/screens/SettingsScreen.kt | 24 ++----- 2 files changed, 46 insertions(+), 45 deletions(-) diff --git a/app/src/main/java/com/pulseloop/PhoneStepManager.kt b/app/src/main/java/com/pulseloop/PhoneStepManager.kt index d2a4a627..aef31fd9 100644 --- a/app/src/main/java/com/pulseloop/PhoneStepManager.kt +++ b/app/src/main/java/com/pulseloop/PhoneStepManager.kt @@ -75,19 +75,7 @@ class PhoneStepManager(private val context: Context) { * the value is unchanged, so re-publishing the same 0 on every foreground is free. */ suspend fun refresh(): Boolean { - val client = try { - HealthConnectClient.getOrCreate(context) - } catch (_: Exception) { - return false - } - - val readStepsPermission = HealthPermission.getReadPermission(StepsRecord::class) - val granted = try { - client.permissionController.getGrantedPermissions() - } catch (_: Exception) { - return false - } - if (readStepsPermission !in granted) return false + val client = clientWithStepsPermission(context) ?: return false val now = Instant.now() val startOfDayMs = TimeUtil.startOfDayLocal(System.currentTimeMillis()) @@ -164,19 +152,7 @@ class PhoneStepManager(private val context: Context) { * @return true if the query succeeded (even if some days were empty), false on failure. */ suspend fun refreshHistoricalDays(daysBack: Long = 30): Boolean { - val client = try { - HealthConnectClient.getOrCreate(context) - } catch (_: Exception) { - return false - } - - val readStepsPermission = HealthPermission.getReadPermission(StepsRecord::class) - val granted = try { - client.permissionController.getGrantedPermissions() - } catch (_: Exception) { - return false - } - if (readStepsPermission !in granted) return false + val client = clientWithStepsPermission(context) ?: return false // Anchor the range to local midnight so each bucket is exactly one calendar day. // endLocal is tomorrow-midnight (exclusive upper bound); startLocal is the first @@ -247,4 +223,43 @@ class PhoneStepManager(private val context: Context) { ).records return records.map { it.metadata.dataOrigin }.toSet() - ownOrigin } + + companion object { + /** + * READ_STEPS permission string, resolved once. Referenced by the reads above (through + * [clientWithStepsPermission]) and by [com.pulseloop.ui.screens.SettingsScreen]'s Step + * Source row (to test whether the permission is already granted before asking, and to + * verify the permission-result callback was granted *this* permission). + */ + val READ_STEPS_PERMISSION: String = + HealthPermission.getReadPermission(StepsRecord::class) + + /** + * The Health Connect client if READ_STEPS is granted, else null. Null means: HC is + * unavailable on this device, the client could not be constructed, the permission + * query threw, or READ_STEPS is simply not granted. Callers in the phone step path + * treat all four the same way — a silent no-op that leaves the ring's own data in + * place. + */ + suspend fun clientWithStepsPermission(context: Context): HealthConnectClient? { + val client = try { + HealthConnectClient.getOrCreate(context) + } catch (_: Exception) { + return null + } + val granted = try { + client.permissionController.getGrantedPermissions() + } catch (_: Exception) { + return null + } + return if (READ_STEPS_PERMISSION in granted) client else null + } + + /** + * True when READ_STEPS is granted — the Settings Step Source row's "already granted" + * check, so its async HC preamble isn't a third copy of [clientWithStepsPermission]'s. + */ + suspend fun hasStepsPermission(context: Context): Boolean = + clientWithStepsPermission(context) != null + } } diff --git a/app/src/main/java/com/pulseloop/ui/screens/SettingsScreen.kt b/app/src/main/java/com/pulseloop/ui/screens/SettingsScreen.kt index a054cc42..75c092d6 100644 --- a/app/src/main/java/com/pulseloop/ui/screens/SettingsScreen.kt +++ b/app/src/main/java/com/pulseloop/ui/screens/SettingsScreen.kt @@ -30,16 +30,6 @@ import kotlinx.coroutines.Job import kotlinx.coroutines.launch import kotlinx.coroutines.withContext -/** - * READ_STEPS permission string, resolved once. Referenced by both the Step Source row's - * "switch to Phone" path (to test whether the permission is already granted before - * asking) and the permission-result launcher's callback (to verify the user actually - * granted *this* permission, not some other one). - */ -private val READ_STEPS_PERMISSION: String = - androidx.health.connect.client.permission.HealthPermission - .getReadPermission(androidx.health.connect.client.records.StepsRecord::class) - /** * Ported from SettingsView.swift (iOS #49 rehaul). * Top-level Settings: a hero ring-device card over grouped sections of navigation rows. Each @@ -103,7 +93,7 @@ fun SettingsScreen( val healthConnectPermissionLauncher = rememberLauncherForActivityResult( androidx.health.connect.client.PermissionController.createRequestPermissionResultContract() ) { granted -> - if (READ_STEPS_PERMISSION in granted) { + if (com.pulseloop.PhoneStepManager.READ_STEPS_PERMISSION in granted) { // Granted — now it is safe to persist "phone" and run the backfill. A6: the // preference is written only after permission is confirmed, so an interrupted // flow cannot leave the app claiming a source it can't read from. @@ -246,13 +236,7 @@ fun SettingsScreen( return@launch } val hasPermission = withContext(Dispatchers.IO) { - try { - val client = androidx.health.connect.client.HealthConnectClient - .getOrCreate(context) - READ_STEPS_PERMISSION in client.permissionController.getGrantedPermissions() - } catch (_: Exception) { - false - } + com.pulseloop.PhoneStepManager.hasStepsPermission(context) } if (hasPermission) { // Re-check intent immediately before persisting: the user @@ -273,7 +257,9 @@ fun SettingsScreen( } else { // Not granted — ask. The launcher's callback persists // "phone" only once permission is confirmed. - healthConnectPermissionLauncher.launch(setOf(READ_STEPS_PERMISSION)) + healthConnectPermissionLauncher.launch( + setOf(com.pulseloop.PhoneStepManager.READ_STEPS_PERMISSION) + ) } } } From afc91a5ef03ed67c165af97952586846a84ca82a Mon Sep 17 00:00:00 2001 From: leona Date: Wed, 7 Oct 2026 22:32:04 +0200 Subject: [PATCH 15/33] Step Source: throttle the automatic backfill after an explicit one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both explicit toggle paths (permission-sheet callback and already-granted handler) ran refreshHistoricalDays() without updating lastBackfillDay, so MainActivity.onResume's refreshPhoneSteps() would see the throttle stale and run the same 30-day backfill a second time — immediately after the sheet in one case, on the next foreground in the other. The toggle itself stays unthrottled: an explicit user action always runs. It now also stamps the throttle, so the automatic path skips the duplicate. --- .../com/pulseloop/ui/screens/SettingsScreen.kt | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/app/src/main/java/com/pulseloop/ui/screens/SettingsScreen.kt b/app/src/main/java/com/pulseloop/ui/screens/SettingsScreen.kt index 75c092d6..0f4ea50b 100644 --- a/app/src/main/java/com/pulseloop/ui/screens/SettingsScreen.kt +++ b/app/src/main/java/com/pulseloop/ui/screens/SettingsScreen.kt @@ -25,6 +25,7 @@ import com.pulseloop.ui.components.DeviceHeroCard import com.pulseloop.ui.components.SettingsRowItem import com.pulseloop.ui.components.SettingsSection import com.pulseloop.ui.theme.PulseColors +import com.pulseloop.util.TimeUtil import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.Job import kotlinx.coroutines.launch @@ -90,6 +91,12 @@ fun SettingsScreen( // point, so a user who declines is not asked again until they touch the toggle // once more. If the dialog is declined, the toggle reverts to "Ring" so the UI // never claims a source the app cannot actually read from. + // + // The explicit backfill here marks `lastBackfillDay` before launching: returning from + // the permission sheet fires MainActivity.onResume, whose automatic refreshPhoneSteps() + // would otherwise see the throttle stale and run the same 30-day backfill a second time. + // The toggle is not throttled — an explicit user action always runs — but it *does* + // update the throttle so the automatic path doesn't immediately repeat the work. val healthConnectPermissionLauncher = rememberLauncherForActivityResult( androidx.health.connect.client.PermissionController.createRequestPermissionResultContract() ) { granted -> @@ -98,6 +105,7 @@ fun SettingsScreen( // preference is written only after permission is confirmed, so an interrupted // flow cannot leave the app claiming a source it can't read from. stepSourcePrefs.stepSource = StepSourcePrefs.SOURCE_PHONE + stepSourcePrefs.lastBackfillDay = TimeUtil.startOfTodayLocal() scope.launch(Dispatchers.IO) { try { com.pulseloop.PhoneStepManager(context).refreshHistoricalDays() @@ -244,8 +252,12 @@ fun SettingsScreen( // was in flight, and cancellation is cooperative — this // coroutine can reach here with the cancel flag set. if (stepSource != StepSourcePrefs.SOURCE_PHONE) return@launch - // Already granted — persist and backfill now. + // Already granted — persist and backfill now. Same throttle + // update as the permission-sheet callback: without it, the + // next onResume's automatic refreshPhoneSteps() would see the + // throttle stale and run this same 30-day backfill again. stepSourcePrefs.stepSource = StepSourcePrefs.SOURCE_PHONE + stepSourcePrefs.lastBackfillDay = TimeUtil.startOfTodayLocal() withContext(Dispatchers.IO) { try { com.pulseloop.PhoneStepManager(context).refreshHistoricalDays() @@ -301,4 +313,4 @@ fun SettingsScreen( Spacer(Modifier.height(32.dp)) } -} +} \ No newline at end of file From ba613fb043417f311d8ea330eb3d3c1d46b814c4 Mon Sep 17 00:00:00 2001 From: leona Date: Wed, 7 Oct 2026 22:37:57 +0200 Subject: [PATCH 16/33] Add trailing newlines to files that lost them Seven files ended without a final newline, showing as a no-newline-at-EOF artifact in the review diff. No code changes. --- app/src/main/AndroidManifest.xml | 2 +- app/src/main/java/com/pulseloop/ring/PulseEventBus.kt | 2 +- .../main/java/com/pulseloop/service/DailyCalorieEstimator.kt | 2 +- app/src/main/java/com/pulseloop/settings/ApiKeyStore.kt | 2 +- app/src/main/java/com/pulseloop/settings/StepSourcePrefs.kt | 2 +- app/src/main/java/com/pulseloop/ui/screens/ActivityScreen.kt | 2 +- app/src/main/java/com/pulseloop/ui/screens/SettingsScreen.kt | 2 +- 7 files changed, 7 insertions(+), 7 deletions(-) diff --git a/app/src/main/AndroidManifest.xml b/app/src/main/AndroidManifest.xml index 2898a355..9cbb15d9 100644 --- a/app/src/main/AndroidManifest.xml +++ b/app/src/main/AndroidManifest.xml @@ -211,4 +211,4 @@ - \ No newline at end of file + diff --git a/app/src/main/java/com/pulseloop/ring/PulseEventBus.kt b/app/src/main/java/com/pulseloop/ring/PulseEventBus.kt index dd2b93b3..49823775 100644 --- a/app/src/main/java/com/pulseloop/ring/PulseEventBus.kt +++ b/app/src/main/java/com/pulseloop/ring/PulseEventBus.kt @@ -219,4 +219,4 @@ object PulseEventBus { android.util.Log.e("PulseEventBus", "Pulse event queue rejected ${event.javaClass.simpleName}", result.exceptionOrNull()) } } -} \ No newline at end of file +} diff --git a/app/src/main/java/com/pulseloop/service/DailyCalorieEstimator.kt b/app/src/main/java/com/pulseloop/service/DailyCalorieEstimator.kt index 2fcf9260..b6cfecb5 100644 --- a/app/src/main/java/com/pulseloop/service/DailyCalorieEstimator.kt +++ b/app/src/main/java/com/pulseloop/service/DailyCalorieEstimator.kt @@ -321,4 +321,4 @@ object DailyCalorieEstimator { * Android pages, and a day can only overlap sessions from that day and its neighbours. */ private const val WORKOUT_SCAN_LIMIT = 50 -} \ No newline at end of file +} diff --git a/app/src/main/java/com/pulseloop/settings/ApiKeyStore.kt b/app/src/main/java/com/pulseloop/settings/ApiKeyStore.kt index f54a1669..930be423 100644 --- a/app/src/main/java/com/pulseloop/settings/ApiKeyStore.kt +++ b/app/src/main/java/com/pulseloop/settings/ApiKeyStore.kt @@ -207,4 +207,4 @@ class ApiKeyStore(context: Context) { private const val KEY_LUNG_CONDITION = "has_known_lung_condition" private const val KEY_GLUCOSE_UNIT = "preferred_glucose_unit" } -} \ No newline at end of file +} diff --git a/app/src/main/java/com/pulseloop/settings/StepSourcePrefs.kt b/app/src/main/java/com/pulseloop/settings/StepSourcePrefs.kt index 94026c8f..1abad3d6 100644 --- a/app/src/main/java/com/pulseloop/settings/StepSourcePrefs.kt +++ b/app/src/main/java/com/pulseloop/settings/StepSourcePrefs.kt @@ -53,4 +53,4 @@ class StepSourcePrefs(context: Context) { private const val KEY_STEP_SOURCE = "step_source" private const val KEY_LAST_BACKFILL_DAY = "last_backfill_day" } -} \ No newline at end of file +} diff --git a/app/src/main/java/com/pulseloop/ui/screens/ActivityScreen.kt b/app/src/main/java/com/pulseloop/ui/screens/ActivityScreen.kt index eb03de91..8a50a5a2 100644 --- a/app/src/main/java/com/pulseloop/ui/screens/ActivityScreen.kt +++ b/app/src/main/java/com/pulseloop/ui/screens/ActivityScreen.kt @@ -910,4 +910,4 @@ private fun ActivityRecordsCard( } /** How many activity records the list shows before asking (issue #70). */ -private const val ACTIVITY_RECORDS_PAGE_SIZE = 50 \ No newline at end of file +private const val ACTIVITY_RECORDS_PAGE_SIZE = 50 diff --git a/app/src/main/java/com/pulseloop/ui/screens/SettingsScreen.kt b/app/src/main/java/com/pulseloop/ui/screens/SettingsScreen.kt index 0f4ea50b..906af079 100644 --- a/app/src/main/java/com/pulseloop/ui/screens/SettingsScreen.kt +++ b/app/src/main/java/com/pulseloop/ui/screens/SettingsScreen.kt @@ -313,4 +313,4 @@ fun SettingsScreen( Spacer(Modifier.height(32.dp)) } -} \ No newline at end of file +} From 1134108813d4b5b334cf360d95f7a09bde10048c Mon Sep 17 00:00:00 2001 From: leona Date: Wed, 7 Oct 2026 22:43:50 +0200 Subject: [PATCH 17/33] Fix PhoneStepSourceTest: EXCLUDED_SOURCES no longer contains phone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 41beee0 dropped "phone" from EXCLUDED_SOURCES — the row's own source field never reaches the exporter under Path B, so a 0-step phone-first day is dropped by the plausibility guards instead. The assertion here still expected the old shape and was failing. The correct assertion (assertFalse) already lives in HealthConnectTypeMappingsTest.excludedSourcesCoversDemoAndMock, added in the same 41beee0. This test is superseded; delete it rather than rewrite a duplicate. --- .../pulseloop/service/PhoneStepSourceTest.kt | 22 ++++++++----------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/app/src/test/java/com/pulseloop/service/PhoneStepSourceTest.kt b/app/src/test/java/com/pulseloop/service/PhoneStepSourceTest.kt index b8be2c4c..0a345060 100644 --- a/app/src/test/java/com/pulseloop/service/PhoneStepSourceTest.kt +++ b/app/src/test/java/com/pulseloop/service/PhoneStepSourceTest.kt @@ -1,11 +1,9 @@ package com.pulseloop.service -import com.pulseloop.health.HealthConnectTypeMappings import com.pulseloop.ring.PulseEvent import com.pulseloop.settings.StepSourcePrefs import org.junit.Assert.assertEquals import org.junit.Assert.assertNotEquals -import org.junit.Assert.assertTrue import org.junit.Test import java.time.Instant @@ -14,23 +12,21 @@ import java.time.Instant * (`PhoneStepManager`) would need a mocked HC client and is out of scope here — these tests * protect the invariants that are easy to break silently: * - * - the exporter must never re-publish a phone row (that closes the self-feedback loop) * - the two source constants stay distinct (a typo collapsing them would silently break the * preference switch) * - the event carries only the step count (adding a hardcoded distance/calorie back would * bypass DailyCalorieEstimator without any compile error) + * + * The read-time source selection itself (`ActivityDailyEntity.displaySteps` / `forDisplay`) is + * covered in `app/src/test/java/com/pulseloop/data/ActivityDailyDisplayTest.kt`. The phone + * write's column-only behaviour is covered (Room-backed) in + * `app/src/test/java/com/pulseloop/service/EventPersistenceSubscriberPhoneWriteTest.kt`. + * The EXCLUDED_SOURCES assertion that used to live here was superseded by + * `HealthConnectTypeMappingsTest.excludedSourcesCoversDemoAndMock` in commit 41beee0 — `"phone"` + * was dropped from the set, so the old assertion was simply wrong. */ class PhoneStepSourceTest { - @Test - fun excludedSourcesContainsPhone() { - assertTrue( - "EXCLUDED_SOURCES must contain \"phone\" so ActivityExporter never writes a " + - "phone-sourced row to Health Connect — that write is the self-feedback loop.", - HealthConnectTypeMappings.EXCLUDED_SOURCES.contains(StepSourcePrefs.SOURCE_PHONE), - ) - } - @Test fun sourceConstantsAreDistinct() { assertNotEquals(StepSourcePrefs.SOURCE_RING, StepSourcePrefs.SOURCE_PHONE) @@ -47,4 +43,4 @@ class PhoneStepSourceTest { ) assertEquals(123, event.steps) } -} +} \ No newline at end of file From 22cdd507b26df45e90c146c204c5f1691c1b3277 Mon Sep 17 00:00:00 2001 From: leona Date: Wed, 7 Oct 2026 22:48:40 +0200 Subject: [PATCH 18/33] Test read-time step-source selection (displaySteps / forDisplay) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Covers the rules the phone/ring display rests on: ring mode always shows the ring column, phone mode shows phoneSteps when set and falls back to steps when null, phoneSteps = 0 is a real state distinct from null, and forDisplay rewrites only steps — leaving calories, distance, and source untouched is what makes switching source reversible. Pure functions, no Room — these are the exact helpers the ViewModels, widgets, and notification context builder call. --- .../data/ActivityDailyDisplayTest.kt | 115 ++++++++++++++++++ 1 file changed, 115 insertions(+) create mode 100644 app/src/test/java/com/pulseloop/data/ActivityDailyDisplayTest.kt diff --git a/app/src/test/java/com/pulseloop/data/ActivityDailyDisplayTest.kt b/app/src/test/java/com/pulseloop/data/ActivityDailyDisplayTest.kt new file mode 100644 index 00000000..d7ca8578 --- /dev/null +++ b/app/src/test/java/com/pulseloop/data/ActivityDailyDisplayTest.kt @@ -0,0 +1,115 @@ +package com.pulseloop.data + +import com.pulseloop.data.entity.ActivityDailyEntity +import org.junit.Assert.assertEquals +import org.junit.Assert.assertSame +import org.junit.Test + +/** + * Read-time step-source selection (PR #98, Path B). The phone never overwrites the ring's + * `steps` column — it writes `phoneSteps` — and the display picks between them here. The rules + * the whole feature rests on: + * + * - Ring mode (`preferPhone = false`) always shows the ring's `steps`, never `phoneSteps`. + * - Phone mode (`preferPhone = true`) shows `phoneSteps` when it is non-null, else falls back + * to the ring's `steps` — a day the phone never backfilled still shows something. + * - `phoneSteps = 0` is a real state distinct from `null`: the phone was counting and counted + * zero. Only `null` means "the phone never wrote for this day". + * - `forDisplay` rewrites only `steps`; every other column (calories, distance, source) is + * left as the ring wrote it — that is what makes switching source reversible. + * + * Pure functions, no Room: these are the same helpers the ViewModels, widgets, and the + * notification context builder call. + */ +class ActivityDailyDisplayTest { + + private fun row( + steps: Int = 5_000, + phoneSteps: Int? = null, + calories: Double = 200.0, + distanceMeters: Double = 4_000.0, + source: String = "ring", + ) = ActivityDailyEntity( + date = 1_700_000_000_000L, + steps = steps, + phoneSteps = phoneSteps, + calories = calories, + distanceMeters = distanceMeters, + source = source, + ) + + // ── displaySteps (scalar read) ── + + @Test + fun ringModeShowsRingStepsEvenWhenPhoneHasAValue() { + assertEquals(5_000, row(phoneSteps = 8_200).displaySteps(preferPhone = false)) + } + + @Test + fun ringModeShowsRingStepsWhenPhoneHasNoValue() { + assertEquals(5_000, row(phoneSteps = null).displaySteps(preferPhone = false)) + } + + @Test + fun phoneModeShowsPhoneStepsWhenSet() { + assertEquals(8_200, row(phoneSteps = 8_200).displaySteps(preferPhone = true)) + } + + @Test + fun phoneModeFallsBackToRingStepsWhenPhoneIsNull() { + assertEquals(5_000, row(phoneSteps = null).displaySteps(preferPhone = true)) + } + + @Test + fun phoneModeShowsZeroWhenPhoneWalkedZero() { + // The midnight-gap case: the phone was counting and counted 0, which must display as + // 0 and not fall through to the ring's stale value. + assertEquals(0, row(steps = 5_000, phoneSteps = 0).displaySteps(preferPhone = true)) + } + + // ── forDisplay (whole-row projection) ── + + @Test + fun forDisplayInRingModeReturnsRowUnchanged() { + val original = row(phoneSteps = 8_200) + assertSame(original, original.forDisplay(preferPhone = false)) + } + + @Test + fun forDisplayInPhoneModeWithNoPhoneValueReturnsRowUnchanged() { + val original = row(phoneSteps = null) + assertSame(original, original.forDisplay(preferPhone = true)) + } + + @Test + fun forDisplayInPhoneModeProjectsPhoneStepsOntoSteps() { + val original = row(steps = 5_000, phoneSteps = 8_200) + val shown = original.forDisplay(preferPhone = true) + assertEquals(8_200, shown.steps) + } + + @Test + fun forDisplayLeavesEveryOtherColumnUntouched() { + // Path B's whole point: only `steps` is projected. If calories/distance ever changed + // under source switching, the ring's real values would be lost. + val original = row( + steps = 5_000, phoneSteps = 8_200, + calories = 210.5, distanceMeters = 4_321.0, source = "ring", + ) + val shown = original.forDisplay(preferPhone = true) + assertEquals(210.5, shown.calories, 0.0) + assertEquals(4_321.0, shown.distanceMeters, 0.0) + assertEquals("ring", shown.source) + assertEquals(8_200, shown.phoneSteps) // phoneSteps itself is unchanged + assertEquals(1_700_000_000_000L, shown.date) + } + + @Test + fun forDisplayZeroPhoneStepsProjectsZeroNotFallback() { + // Same as the midnight-gap scalar case, but through the whole-row path the ViewModels + // actually use. + val original = row(steps = 5_000, phoneSteps = 0) + val shown = original.forDisplay(preferPhone = true) + assertEquals(0, shown.steps) + } +} From e19c7329ac56f2a698c3c1952ce0680a7517b6c1 Mon Sep 17 00:00:00 2001 From: leona Date: Wed, 7 Oct 2026 23:00:21 +0200 Subject: [PATCH 19/33] Test the phone write's column-only contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The phone write touches exactly one column of an existing row — that is what makes the source switch reversible and what stops the phone from clobbering the ring's calories and distance. Extracted the row decision into a pure helper (phoneStepRow) so it can be tested without a Room instance; the surrounding upsert is now one line. Covers: only phoneSteps/syncedAt/updatedAt change on an existing row, a brand-new row is created with source=phone, writing the same value twice is a no-op, and phoneSteps=0 is a real value distinct from null (the midnight-gap case). --- .../service/EventPersistenceSubscriber.kt | 44 ++++++--- .../service/EventPersistencePhoneWriteTest.kt | 97 +++++++++++++++++++ 2 files changed, 130 insertions(+), 11 deletions(-) create mode 100644 app/src/test/java/com/pulseloop/service/EventPersistencePhoneWriteTest.kt diff --git a/app/src/main/java/com/pulseloop/service/EventPersistenceSubscriber.kt b/app/src/main/java/com/pulseloop/service/EventPersistenceSubscriber.kt index 79622435..1dce0bdd 100644 --- a/app/src/main/java/com/pulseloop/service/EventPersistenceSubscriber.kt +++ b/app/src/main/java/com/pulseloop/service/EventPersistenceSubscriber.kt @@ -569,17 +569,7 @@ class EventPersistenceSubscriber( private suspend fun upsertActivityDailyFromPhone(ts: Long, steps: Int) { val dayStart = com.pulseloop.util.TimeUtil.startOfDayLocal(ts) val existing = db.activityDailyDao().byDay(dayStart) - - if (existing != null && existing.phoneSteps == steps) return - - val now = System.currentTimeMillis() - db.activityDailyDao().upsert( - (existing ?: ActivityDailyEntity(date = dayStart, source = StepSourcePrefs.SOURCE_PHONE)).copy( - phoneSteps = steps, - syncedAt = now, - updatedAt = now, - ) - ) + phoneStepRow(existing, dayStart, steps)?.let { db.activityDailyDao().upsert(it) } } /** @@ -895,6 +885,38 @@ class EventPersistenceSubscriber( } } +/** + * The row a [PulseEvent.PhoneStepsUpdate] should write for one day — or null when it would be a + * no-op. + * + * Pure, so the phone write's column-only contract can be tested without a Room instance (same + * pattern as the sleep-merge helpers below). The contract, all of which is asserted in + * `EventPersistencePhoneWriteTest`: + * + * - **Only three columns change on an existing row:** `phoneSteps`, `syncedAt`, `updatedAt`. + * `steps`, `calories`, `distanceMeters`, and `source` are carried through untouched — that is + * what makes switching step source reversible. + * - **A brand-new row is created with `source = "phone"`**, so the day is visible in the "real" + * DAO queries even before the ring has data for it. + * - **Writing the same value twice is a no-op** (returns null): the phone reader re-publishes + * today's total on every foreground, and without this guard each pass would bump `updatedAt` + * — which `ActivityExporter` selects by — and re-export the same record. + * - **`phoneSteps = 0` is a real value, distinct from null.** `null → 0` writes (the midnight + * gap: the phone has begun counting and counted zero); `0 → 0` is a no-op. + */ +internal fun phoneStepRow( + existing: ActivityDailyEntity?, + dayStart: Long, + steps: Int, + now: Long = System.currentTimeMillis(), +): ActivityDailyEntity? = + if (existing != null && existing.phoneSteps == steps) null + else (existing ?: ActivityDailyEntity(date = dayStart, source = StepSourcePrefs.SOURCE_PHONE)).copy( + phoneSteps = steps, + syncedAt = now, + updatedAt = now, + ) + /** * Is this reading one the ring will hand back from its own history, so that the two copies have to * be reconciled later ([EventPersistenceSubscriber.adoptRingsCopy])? Both halves are required: diff --git a/app/src/test/java/com/pulseloop/service/EventPersistencePhoneWriteTest.kt b/app/src/test/java/com/pulseloop/service/EventPersistencePhoneWriteTest.kt new file mode 100644 index 00000000..b329187c --- /dev/null +++ b/app/src/test/java/com/pulseloop/service/EventPersistencePhoneWriteTest.kt @@ -0,0 +1,97 @@ +package com.pulseloop.service + +import com.pulseloop.data.entity.ActivityDailyEntity +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNotNull +import org.junit.Assert.assertNull +import org.junit.Test + +/** + * The phone step write's column-only contract (PR #98, Path B). Phone and ring data live in + * separate columns, and the phone write touches exactly one of them — that is what makes the + * source switch reversible and what stops the phone from clobbering the ring's calories and + * distance. [phoneStepRow] is the pure decision this rests on (same pattern as + * `EventPersistenceIdentityTest`'s helpers); the Room call around it is one line. + * + * The four rules asserted here: + * - only `phoneSteps`/`syncedAt`/`updatedAt` change on an existing row + * - a brand-new row is created with `source = "phone"` + * - writing the same value twice is a no-op + * - `phoneSteps = 0` is a real value distinct from null + */ +class EventPersistencePhoneWriteTest { + + private val dayStart = 1_700_000_000_000L + + private fun ringRow( + steps: Int = 5_000, + phoneSteps: Int? = null, + calories: Double = 210.5, + distanceMeters: Double = 4_321.0, + source: String = "ring", + ) = ActivityDailyEntity( + date = dayStart, + steps = steps, + phoneSteps = phoneSteps, + calories = calories, + distanceMeters = distanceMeters, + source = source, + ) + + @Test + fun phoneWriteOnlyTouchesPhoneStepsSyncedAtAndUpdatedAt() { + val existing = ringRow() + val written = phoneStepRow(existing, dayStart, steps = 8_200, now = 999L)!! + + assertEquals(8_200, written.phoneSteps) + assertEquals(999L, written.syncedAt) + assertEquals(999L, written.updatedAt) + + // Ring's own columns, untouched: + assertEquals(5_000, written.steps) + assertEquals(210.5, written.calories, 0.0) + assertEquals(4_321.0, written.distanceMeters, 0.0) + assertEquals("ring", written.source) + } + + @Test + fun firstPhoneWriteOnADayCreatesRowWithPhoneSource() { + val written = phoneStepRow(null, dayStart = dayStart, steps = 8_200, now = 999L)!! + + assertEquals(dayStart, written.date) + assertEquals(8_200, written.phoneSteps) + assertEquals("phone", written.source) + assertEquals(0, written.steps) // default — nothing has written the ring column yet + assertEquals(0.0, written.calories, 0.0) // default + assertEquals(0.0, written.distanceMeters, 0.0) + } + + @Test + fun sameValueTwiceIsANoOp() { + // `null` returned means "don't write" — the caller skips the upsert entirely. + assertNull(phoneStepRow(ringRow(phoneSteps = 8_200), dayStart, steps = 8_200)) + assertNull(phoneStepRow(ringRow(phoneSteps = 0), dayStart, steps = 0)) + } + + @Test + fun differentValueWrites() { + // 8_200 → 8_500: today's phone count moved; a write is required. + assertNotNull(phoneStepRow(ringRow(phoneSteps = 8_200), dayStart, steps = 8_500)) + } + + @Test + fun nullToZeroWrites() { + // The midnight-gap case: phone has begun counting and counted zero. This is the write + // that makes "today's phone value is 0" a real state — skipping it would leave the day's + // phoneSteps null and the display showing the ring's stale total. + val written = phoneStepRow(ringRow(phoneSteps = null), dayStart, steps = 0)!! + assertEquals(0, written.phoneSteps) + } + + @Test + fun zeroToNullWouldWrite() { + // Not a state we expect in production, but the helper's contract must not confuse + // "explicit zero" with "no value": only equal-non-null values are the no-op case. + assertNotNull(phoneStepRow(ringRow(phoneSteps = 0), dayStart, steps = 1)) + } +} From 6ccd2782756e4a765cce1f4dbd4aac64c3d739be Mon Sep 17 00:00:00 2001 From: leona Date: Wed, 7 Oct 2026 23:03:06 +0200 Subject: [PATCH 20/33] EventPersistenceSubscriber: fix stale EXCLUDED_SOURCES KDocs Two comments still described the pre-41beee0 design, where "phone" lived in EXCLUDED_SOURCES to break a self-feedback loop. The exclusion is gone; phone-first days produce no Health Connect record via the isPlausibleSteps floor instead. Also tightens the phone-write KDoc's "one column only" claim: syncedAt and updatedAt do move, they're just metadata rather than data columns. --- .../service/EventPersistenceSubscriber.kt | 42 ++++++++++--------- 1 file changed, 22 insertions(+), 20 deletions(-) diff --git a/app/src/main/java/com/pulseloop/service/EventPersistenceSubscriber.kt b/app/src/main/java/com/pulseloop/service/EventPersistenceSubscriber.kt index 1dce0bdd..7f18008b 100644 --- a/app/src/main/java/com/pulseloop/service/EventPersistenceSubscriber.kt +++ b/app/src/main/java/com/pulseloop/service/EventPersistenceSubscriber.kt @@ -509,12 +509,13 @@ class EventPersistenceSubscriber( val hasDeletion = existing.deletedSteps > 0 || existing.deletedDistanceMeters > 0.0 db.activityDailyDao().upsert(existing.copy( // A row the phone created first (`source == "phone"`) flips to `"ring"` when the - // ring writes — the `source` column still has two consumers downstream: the - // Health Connect exporter (via `HealthConnectTypeMappings.EXCLUDED_SOURCES`) and - // [DailyCalorieEstimator.deviceReportedCalories]. Both read it as "who last wrote - // these columns", so once the ring has supplied `steps`/`calories`/`distance`, - // the row belongs to the ring. Other sources ("ring_history", "manual_recording", - // "hr_and_manual") are preserved as before. + // ring writes. The `source` column still has two downstream consumers — + // `HealthConnectTypeMappings.EXCLUDED_SOURCES` (which filters demo/mock, not + // phone) and [DailyCalorieEstimator.deviceReportedCalories] — but only the latter + // cares about `"phone"`, and it reads the column as "who last wrote these data + // columns". So once the ring has supplied `steps`/`calories`/`distance`, the row + // belongs to the ring and the estimator must see it that way. Other sources + // ("ring_history", "manual_recording", "hr_and_manual") are preserved as before. source = if (existing.source == StepSourcePrefs.SOURCE_PHONE) StepSourcePrefs.SOURCE_RING else existing.source, steps = if (stale) steps else deletion.ratchetAgainstRing(existing.steps, steps, existing.deletedSteps), @@ -541,22 +542,23 @@ class EventPersistenceSubscriber( /** * Persist the phone's step count for one day, in its own column. * - * Under the separate-column design ([ActivityDailyEntity.phoneSteps]), an existing row is - * updated in **one column only**: `phoneSteps`. `steps`, `distanceMeters`, `calories`, and - * `source` are left exactly as the ring wrote them. The display picks between `steps` and - * `phoneSteps` at read time based on the user's step-source preference, so switching source - * is a read-time choice, not a destructive write: the ring's totals are never clobbered and - * switching back is instant. + * Under the separate-column design ([ActivityDailyEntity.phoneSteps]), an existing row's + * **data columns** are untouched except for `phoneSteps`: `steps`, `distanceMeters`, + * `calories`, and `source` are left exactly as the ring wrote them. (`syncedAt` and + * `updatedAt` also move — metadata, not data; `updatedAt` is what ActivityExporter selects + * on.) The display picks between `steps` and `phoneSteps` at read time based on the user's + * step-source preference, so switching source is a read-time choice, not a destructive + * write: the ring's totals are never clobbered and switching back is instant. * * A brand-new row (phone wrote first, ring hasn't yet) is created with - * `source = "phone"` for two reasons: - * - so the day is visible in the "real" DAO queries - * (`source NOT IN ('demo','mock')`) even before the ring has data for it, and - * - so the row — which has `steps = 0` — stays out of the Health Connect export via - * [com.pulseloop.health.HealthConnectTypeMappings.EXCLUDED_SOURCES]. The old design - * needed that exclusion to break a self-feedback loop (the phone's value used to land - * in `steps`, the exported column); under the separate-column design the loop is broken - * by construction, and the exclusion is now only about not exporting a 0-step record. + * `source = "phone"` so the day is visible in the "real" DAO queries + * (`source NOT IN ('demo','mock')`) even before the ring has data for it. The old design + * also needed the source label to keep the row out of the Health Connect export, via + * `HealthConnectTypeMappings.EXCLUDED_SOURCES` — that exclusion is gone (the phone's value + * used to land in `steps`, the exported column; under the separate-column design the + * self-feedback loop is broken by construction). A phone-first day still produces no HC + * record: it carries `steps = 0`, and `isPlausibleSteps` floors at 1, so the plausibility + * guards skip it regardless of source. * * Later ring writes preserve that source — see [upsertActivityDaily]. The old "overwrite * source and reset it back to ring on next ring write" dance is gone; nothing needs it any From f7013933533e75851405bd84541ff151b9a02cc5 Mon Sep 17 00:00:00 2001 From: leona Date: Sun, 25 Oct 2026 20:37:51 +0100 Subject: [PATCH 21/33] Backup/restore: carry phoneSteps through the archive Review item 6. ActivityDailyDTO gained no field for the phoneSteps column added in c96bd78, so a restore nulled every phone-sourced day. Days older than the 30-day backfill window could not be refilled. --- app/src/main/java/com/pulseloop/data/DataArchive.kt | 7 +++++++ app/src/main/java/com/pulseloop/data/DataArchiveService.kt | 2 ++ 2 files changed, 9 insertions(+) diff --git a/app/src/main/java/com/pulseloop/data/DataArchive.kt b/app/src/main/java/com/pulseloop/data/DataArchive.kt index 949e02b1..e81e7e76 100644 --- a/app/src/main/java/com/pulseloop/data/DataArchive.kt +++ b/app/src/main/java/com/pulseloop/data/DataArchive.kt @@ -74,6 +74,13 @@ data class PulseArchive( * buckets back into the day. */ val deletedSteps: Int = 0, val deletedDistanceMeters: Double = 0.0, val estimatedActiveCalories: Double? = null, + /** PR #98 (Path B): phone-sourced steps, kept separate from the ring's [steps]. Defaulted + * to `null` for two reasons. An archive written before the column existed deserializes as + * "no phone data", which is exactly what [ActivityDailyEntity.displaySteps] already means + * by a missing value. A default of `0` would falsely mean "the phone reported zero", and + * after a restore that would hide the ring's own count on every historical day in Phone + * mode. */ + val phoneSteps: Int? = null, ) @Serializable data class ActivityBucketDTO( diff --git a/app/src/main/java/com/pulseloop/data/DataArchiveService.kt b/app/src/main/java/com/pulseloop/data/DataArchiveService.kt index 4e1c778e..f985d12e 100644 --- a/app/src/main/java/com/pulseloop/data/DataArchiveService.kt +++ b/app/src/main/java/com/pulseloop/data/DataArchiveService.kt @@ -82,6 +82,7 @@ object DataArchiveService { estimatedActiveCalories = c.dblOrNull("estimatedActiveCalories"), deletedSteps = c.int_("deletedSteps"), deletedDistanceMeters = c.dbl("deletedDistanceMeters"), + phoneSteps = c.intOrNull("phoneSteps"), ) }, activityBuckets = collect("activity_buckets") { c -> @@ -399,6 +400,7 @@ object DataArchiveService { estimatedActiveCalories = a.estimatedActiveCalories, deletedSteps = a.deletedSteps, deletedDistanceMeters = a.deletedDistanceMeters, + phoneSteps = a.phoneSteps, )) } for (b in archive.activityBuckets) { From b879868bb98cb4c5f0993b0c79015b0bd8120ec4 Mon Sep 17 00:00:00 2001 From: leona Date: Sun, 25 Oct 2026 20:55:29 +0100 Subject: [PATCH 22/33] Persist ring events from Application.onCreate, not the composition MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review items 1 and 2. Persistence used to start from PulseLoopApp's LaunchedEffect, but MainActivity.onResume's refreshPhoneSteps runs before that LaunchedEffect dispatches — so today's PhoneStepsUpdate and every backfill event went to DiagnosticsSubscriber alone, and the first cold launch of the day lost the backfill. EventPersistenceSubscriber.start now subscribes synchronously (CoroutineStart.UNDISPATCHED) and is called from PulseLoopApplication.onCreate. That fixes item 2 as well: lastBackfillDay is set when events are queued, and once the queue is guaranteed to reach persistence, 'queued' means 'will be saved'. The PulseEventBus hold (commit 06460a9) becomes redundant: it covered the same window, but the window no longer exists. Removing it does not lose events — Application.onCreate's subscribers attach before any real publisher runs — and the comment now records the invariant rather than a workaround. --- .../java/com/pulseloop/PulseLoopApplication.kt | 7 +++++++ .../java/com/pulseloop/ring/PulseEventBus.kt | 16 +++++++--------- .../service/EventPersistenceSubscriber.kt | 7 ++++++- .../main/java/com/pulseloop/ui/PulseLoopApp.kt | 5 +++-- 4 files changed, 23 insertions(+), 12 deletions(-) diff --git a/app/src/main/java/com/pulseloop/PulseLoopApplication.kt b/app/src/main/java/com/pulseloop/PulseLoopApplication.kt index abceff5f..1007247c 100644 --- a/app/src/main/java/com/pulseloop/PulseLoopApplication.kt +++ b/app/src/main/java/com/pulseloop/PulseLoopApplication.kt @@ -31,6 +31,13 @@ class PulseLoopApplication : Application() { QuietHoursPrefs.migrateIndependentBedtime(this) // Application-scoped so capture runs whether or not the Debug screen is open. DiagnosticsSubscriber(PulseLoopDatabase.getInstance(this)).start(appScope) + // Persistence subscribes from here — not from PulseLoopApp's LaunchedEffect — so that + // nothing published after the Activity appears (MainActivity.onResume → refreshPhoneSteps + // → PulseEventBus.publishBlocking) can reach a bus with no persister attached. Before this, + // the phone read raced the composition's LaunchedEffect and today's PhoneStepsUpdate (and + // every backfill event) went to Diagnostics alone. Review item 1, PR #98. Idempotent: + // PulseLoopApp's LaunchedEffect no longer calls persistence.start(). + AppServices.get(this).persistence.start() // One-time cleanup of activity totals corrupted by the old bucket handling; runs off the // main thread and is prefs-gated so it executes once. Safe to race the first sync — the // ring can't connect before this completes a couple of DELETE statements. diff --git a/app/src/main/java/com/pulseloop/ring/PulseEventBus.kt b/app/src/main/java/com/pulseloop/ring/PulseEventBus.kt index 49823775..66fd0f60 100644 --- a/app/src/main/java/com/pulseloop/ring/PulseEventBus.kt +++ b/app/src/main/java/com/pulseloop/ring/PulseEventBus.kt @@ -8,7 +8,6 @@ import kotlinx.coroutines.channels.Channel import kotlinx.coroutines.flow.MutableSharedFlow import kotlinx.coroutines.flow.SharedFlow import kotlinx.coroutines.flow.asSharedFlow -import kotlinx.coroutines.flow.first import kotlinx.coroutines.launch /** @@ -185,14 +184,13 @@ object PulseEventBus { init { dispatchScope.launch { - // Hold events until at least one subscriber is attached. On cold start, - // MainActivity.onResume runs refreshPhoneSteps() before PulseLoopApp's - // LaunchedEffect has called persistence.start() — the composition hasn't run - // yet. With replay = 0, events published in that window are emitted to zero - // subscribers and lost, including today's PhoneStepsUpdate and every backfill - // event. Waiting here defers the drain until the subscriber exists; once it - // does, the whole backlog is delivered in order. - _events.subscriptionCount.first { it > 0 } + // The drain runs from process start. Subscribers must attach by the time + // Application.onCreate returns — DiagnosticsSubscriber and (since PR #98) + // EventPersistenceSubscriber both do. Every real publisher — ring, phone, sync — runs + // after that, so no event published by the app can reach a drain with no subscriber. + // A publisher added inside Application.onCreate *before* those subscribers start + // would lose its event here; if that ever becomes necessary, the fix is to attach the + // subscribers first, not to hold this drain. // The bus is process-long and single-drained: an uncaught throw here would kill the // dispatcher and silently stop every subscriber for the rest of the process (the // SupervisorJob does not restart it). Isolate each emit so one bad event can't do that. diff --git a/app/src/main/java/com/pulseloop/service/EventPersistenceSubscriber.kt b/app/src/main/java/com/pulseloop/service/EventPersistenceSubscriber.kt index 7f18008b..716ebced 100644 --- a/app/src/main/java/com/pulseloop/service/EventPersistenceSubscriber.kt +++ b/app/src/main/java/com/pulseloop/service/EventPersistenceSubscriber.kt @@ -55,7 +55,12 @@ class EventPersistenceSubscriber( fun start() { if (job != null) return - job = scope.launch { + // UNDISPATCHED: the collect call registers its subscriber synchronously, before start() + // returns. Without this, `scope.launch` merely schedules the coroutine and there is still + // a window between start() returning and the subscriber attaching — which is exactly the + // window MainActivity.onResume → refreshPhoneSteps publishes into on a cold start. + // Review item 1, PR #98. Idempotent (the `if (job != null)` guard above). + job = scope.launch(start = CoroutineStart.UNDISPATCHED) { PulseEventBus.events.collect { event -> persist(event) } } } diff --git a/app/src/main/java/com/pulseloop/ui/PulseLoopApp.kt b/app/src/main/java/com/pulseloop/ui/PulseLoopApp.kt index ed10000d..244b91e2 100644 --- a/app/src/main/java/com/pulseloop/ui/PulseLoopApp.kt +++ b/app/src/main/java/com/pulseloop/ui/PulseLoopApp.kt @@ -196,8 +196,9 @@ fun PulseLoopApp() { } } - // Start services - persistence.start() + // Start services. Persistence is already started from PulseLoopApplication.onCreate + // (review item 1, PR #98): it must be subscribed before MainActivity.onResume's + // refreshPhoneSteps publishes. batteryAlerts.start() coordinator.start() summaryCoordinator.start() From afc91d0c42e0509712dae4ff6fbe260ac32be6c6 Mon Sep 17 00:00:00 2001 From: leona Date: Sun, 25 Oct 2026 21:01:21 +0100 Subject: [PATCH 23/33] Phone steps: publish 0 only after a source has proven it writes steps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review item 3. refresh() previously published today's total including 0 unconditionally — correct when the phone has a step writer but wrong when it has none, because it painted a false zero over the ring's real day. StepSourcePrefs.hasEverSeenPhoneSteps is set the first time refresh() or refreshHistoricalDays() sees a foreign Health Connect origin; refresh() then publishes 0 for today only when that flag is set, and otherwise leaves phoneSteps null so the display falls back to the ring. The flag is never reset: whether a source has ever existed is installation-level history, not current-preference state. --- .../java/com/pulseloop/PhoneStepManager.kt | 26 ++++++++++++++++--- .../com/pulseloop/settings/StepSourcePrefs.kt | 21 +++++++++++++++ 2 files changed, 43 insertions(+), 4 deletions(-) diff --git a/app/src/main/java/com/pulseloop/PhoneStepManager.kt b/app/src/main/java/com/pulseloop/PhoneStepManager.kt index aef31fd9..505c88ff 100644 --- a/app/src/main/java/com/pulseloop/PhoneStepManager.kt +++ b/app/src/main/java/com/pulseloop/PhoneStepManager.kt @@ -52,6 +52,9 @@ import java.time.ZoneId */ class PhoneStepManager(private val context: Context) { + /** User prefs including [StepSourcePrefs.hasEverSeenPhoneSteps], which review item 3 relies on. */ + private val prefs = com.pulseloop.settings.StepSourcePrefs(context) + /** * PulseLoop's own origin, as Health Connect identifies it. Excluded from the origins * discovered for a read — see the class KDoc. @@ -91,12 +94,24 @@ class PhoneStepManager(private val context: Context) { } val steps = if (foreignOrigins.isEmpty()) { - // No foreign origin has written steps today yet — a normal state just after - // midnight, and also the permanent state on a phone with no step writer. Either - // way today's phone total is 0. `aggregate` with an empty `dataOriginFilter` - // means "all origins", not "none", so it must not be called here. + // No foreign origin has written steps today. Two sub-cases, and they are not the + // same thing (review item 3, PR #98): + // - the phone has a step writer and it simply hasn't counted anything yet today + // (just after midnight) — publish 0, because that is the honest phone total; + // - the phone has never had a step writer at all — publishing 0 would paint a + // false zero over a real ring day. Leave `phoneSteps` null so the display + // falls back to the ring. + // `hasEverSeenPhoneSteps` distinguishes them. `aggregate` with an empty + // `dataOriginFilter` means "all origins", not "none", so it must not be called + // here regardless. + if (!prefs.hasEverSeenPhoneSteps) return true 0 } else { + // At least one foreign origin writes steps — record the fact so the midnight-gap + // path above keeps publishing 0 on later passes, even if this origin stops + // writing (a source uninstalled but the phone still in Phone mode: the days + // where it did write are real data). + prefs.hasEverSeenPhoneSteps = true try { client.aggregate( AggregateRequest( @@ -171,6 +186,9 @@ class PhoneStepManager(private val context: Context) { return false } if (foreignOrigins.isEmpty()) return true + // A backfill window with a foreign writer proves a step source exists — see + // [StepSourcePrefs.hasEverSeenPhoneSteps]. Same signal as the today read. + prefs.hasEverSeenPhoneSteps = true val buckets = try { client.aggregateGroupByPeriod( diff --git a/app/src/main/java/com/pulseloop/settings/StepSourcePrefs.kt b/app/src/main/java/com/pulseloop/settings/StepSourcePrefs.kt index 1abad3d6..cb88af8f 100644 --- a/app/src/main/java/com/pulseloop/settings/StepSourcePrefs.kt +++ b/app/src/main/java/com/pulseloop/settings/StepSourcePrefs.kt @@ -41,6 +41,26 @@ class StepSourcePrefs(context: Context) { get() = prefs.getLong(KEY_LAST_BACKFILL_DAY, 0L) set(value) { prefs.edit().putLong(KEY_LAST_BACKFILL_DAY, value).apply() } + /** + * True once any Health Connect source has been observed writing steps for this device — + * either a foreign origin seen in a today-read ([com.pulseloop.PhoneStepManager.refresh]) + * or one seen in a backfill window ([com.pulseloop.PhoneStepManager.refreshHistoricalDays]). + * + * Review item 3, PR #98. Without this flag the phone reader cannot tell "today is 0 + * because it's 3 a.m. and the phone has counted nothing yet" (a real state — publish 0 so + * phone mode shows 0) from "today is 0 because no app on this phone writes steps to Health + * Connect at all" (not a real state — publishing 0 would paint a false zero over a real + * ring day). With it: `refresh()` publishes 0 only when [hasEverSeenPhoneSteps] is true. + * When false, the day's `phoneSteps` stays null and the display falls back to the ring. + * + * Never reset. Whether a source has ever existed for this install is installation-level + * history, not current-preference state — a user who toggles to Ring and back still expects + * Phone mode to show real numbers on a phone that has been tracking steps all along. + */ + var hasEverSeenPhoneSteps: Boolean + get() = prefs.getBoolean(KEY_HAS_EVER_SEEN_PHONE_STEPS, false) + set(value) { prefs.edit().putBoolean(KEY_HAS_EVER_SEEN_PHONE_STEPS, value).apply() } + companion object { /** * The two valid values of [stepSource]. Every call site that reads or writes the @@ -52,5 +72,6 @@ class StepSourcePrefs(context: Context) { private const val KEY_STEP_SOURCE = "step_source" private const val KEY_LAST_BACKFILL_DAY = "last_backfill_day" + private const val KEY_HAS_EVER_SEEN_PHONE_STEPS = "has_ever_seen_phone_steps" } } From 98bd96eb9e216d6931c6241d71f934a8db00cf18 Mon Sep 17 00:00:00 2001 From: leona Date: Sun, 25 Oct 2026 21:17:04 +0100 Subject: [PATCH 24/33] Phone fallback: clear today's phoneSteps when the reader fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review item 4. PhoneStepSourceUnavailable was -> Unit on the assumption that the display's read-time pick would fall back to the ring on its own. It does, but only when phoneSteps is null — and a permission revoked mid-day leaves the last phone value in place, so Today, the widget and check-ins showed a stale phone count for the rest of the day while the ring's live count climbed unseen. EventPersistenceSubscriber now handles the event by clearing today's phoneSteps via clearPhoneStepsOnRow. Historical days keep their phoneSteps: a permission loss today does not retract data that was real when collected. clearPhoneStepsOnRow deliberately does not bump updatedAt: ActivityExporter selects rows by it, so a metadata-only change here would re-select the row and re-export unchanged ring steps, calories and distance. That is review item 10's rule applied proactively so this commit does not create a bug item 10 will have to undo. --- .../main/java/com/pulseloop/MainActivity.kt | 15 ++++--- .../java/com/pulseloop/ring/PulseEventBus.kt | 10 +++-- .../service/EventPersistenceSubscriber.kt | 42 ++++++++++++++++-- .../service/EventPersistencePhoneWriteTest.kt | 43 ++++++++++++++++++- 4 files changed, 94 insertions(+), 16 deletions(-) diff --git a/app/src/main/java/com/pulseloop/MainActivity.kt b/app/src/main/java/com/pulseloop/MainActivity.kt index 4c994467..00570b1a 100644 --- a/app/src/main/java/com/pulseloop/MainActivity.kt +++ b/app/src/main/java/com/pulseloop/MainActivity.kt @@ -213,10 +213,11 @@ class MainActivity : ComponentActivity() { * launch nor return-to-foreground re-triggers a dialog the user may have * already declined. * - * On failure, publishes [PulseEvent.PhoneStepSourceUnavailable] so the - * persistence layer can fall back to the ring (A5) — a phone mode that - * cannot actually read steps must not silently gate the ring's own - * totals off forever. + * On failure, publishes [PulseEvent.PhoneStepSourceUnavailable]. The + * persistence layer responds by clearing *today's* `phoneSteps` on the + * `activity_daily` row, so the display's read-time pick falls back to + * the ring's live count instead of leaving a stale phone value on screen + * for the rest of the day. Historical days keep their `phoneSteps`. */ private fun refreshPhoneSteps() { val prefs = StepSourcePrefs(this) @@ -225,10 +226,10 @@ class MainActivity : ComponentActivity() { lifecycleScope.launch(Dispatchers.IO) { val manager = PhoneStepManager(this@MainActivity) if (!manager.refresh()) { - // A5: the phone reader couldn't produce a value — permission revoked, the + // The phone reader couldn't produce a value — permission revoked, the // provider uninstalled, or a device with no Health Connect. Signal the - // persistence layer so it stops gating the ring's live totals off; without - // this, the preference staying on "phone" would freeze today's count. + // persistence layer so it clears today's phoneSteps and the display falls + // back to the ring's live count instead of a stale phone value. com.pulseloop.ring.PulseEventBus.publishBlocking( com.pulseloop.ring.PulseEvent.PhoneStepSourceUnavailable ) diff --git a/app/src/main/java/com/pulseloop/ring/PulseEventBus.kt b/app/src/main/java/com/pulseloop/ring/PulseEventBus.kt index 66fd0f60..8ba720ca 100644 --- a/app/src/main/java/com/pulseloop/ring/PulseEventBus.kt +++ b/app/src/main/java/com/pulseloop/ring/PulseEventBus.kt @@ -90,10 +90,12 @@ sealed class PulseEvent { /** * The phone step reader failed to produce a value this pass — Health Connect unreachable, * or READ_STEPS not granted. Published by [com.pulseloop.MainActivity.refreshPhoneSteps] - * on a `refresh()` false return, so [com.pulseloop.service.EventPersistenceSubscriber] can - * stop gating the ring's live totals off (see its `stepSourceIsPhone`) and let the ring - * fill in until the phone reader recovers. Without this, a revoked permission would drop - * the ring's contributions indefinitely while the preference still read "phone". + * on a `refresh()` false return. [com.pulseloop.service.EventPersistenceSubscriber] + * handles it by clearing *today's* `phoneSteps` on the `activity_daily` row, so the + * display's read-time pick (`phoneSteps ?: steps`) falls back to the ring's live count + * instead of showing whatever stale phone value was last written. Historical days keep + * their `phoneSteps` — that data was real when collected, and a permission loss today + * does not retract it. */ data object PhoneStepSourceUnavailable : PulseEvent() /** diff --git a/app/src/main/java/com/pulseloop/service/EventPersistenceSubscriber.kt b/app/src/main/java/com/pulseloop/service/EventPersistenceSubscriber.kt index 716ebced..8bf99cda 100644 --- a/app/src/main/java/com/pulseloop/service/EventPersistenceSubscriber.kt +++ b/app/src/main/java/com/pulseloop/service/EventPersistenceSubscriber.kt @@ -403,10 +403,17 @@ class EventPersistenceSubscriber( is PulseEvent.PhoneStepsUpdate -> { upsertActivityDailyFromPhone(event.timestamp.toEpochMilli(), event.steps) } - // The phone reader just failed. Under Path B, nothing to do here: the ring writes its - // own column regardless, and the read-time pick falls back to `steps` when a day has - // no `phoneSteps` value. The event still exists on the bus for the debug screen. - is PulseEvent.PhoneStepSourceUnavailable -> Unit + // The phone reader just failed (permission revoked mid-day, provider gone, HC + // unavailable). Clear *today's* phoneSteps so the display's read-time pick falls + // back to the ring's live `steps` — otherwise the last value the phone wrote + // stays on the Today tile, widget and check-ins for the rest of the day while + // the ring's own count climbs unseen. Historical days keep their phoneSteps: a + // permission loss today does not retract data that was real when collected. + is PulseEvent.PhoneStepSourceUnavailable -> { + val dayStart = com.pulseloop.util.TimeUtil.startOfTodayLocal() + val existing = db.activityDailyDao().byDay(dayStart) + clearPhoneStepsOnRow(existing)?.let { db.activityDailyDao().upsert(it) } + } is PulseEvent.SleepTimeline -> { upsertSleepSession(event.timestamp.toEpochMilli(), event.stages, event.completeSession) } @@ -924,6 +931,33 @@ internal fun phoneStepRow( updatedAt = now, ) +/** + * The row a [PulseEvent.PhoneStepSourceUnavailable] should write for today — or null when it + * would be a no-op. + * + * Review item 4, PR #98. When the phone reader fails mid-day (permission revoked, provider + * uninstalled, HC unreachable), the last `phoneSteps` value it wrote stays on the row and the + * display — which picks `phoneSteps ?: steps` — keeps showing it, hiding the ring's live count + * for the rest of the day. Clearing today's `phoneSteps` restores the fallback. + * + * Pure, so the contract can be tested without Room (same pattern as [phoneStepRow]). + * + * The contract: + * - **Returns null when the row is absent** — nothing to clear. `source = "phone"` on a + * phone-first row is preserved (see [phoneStepRow]'s KDoc for why that matters). + * - **Returns null when `phoneSteps` is already null** — the row already falls back to the + * ring. Repeated `PhoneStepSourceUnavailable` events are common (every failed foreground), + * and each one must be a no-op after the first. + * - **Returns a copy with only `phoneSteps = null`** — `steps`, `calories`, `distanceMeters`, + * `source`, `syncedAt` and `updatedAt` are untouched. Not bumping `updatedAt` is deliberate: + * [com.pulseloop.health.exporters.ActivityExporter] selects rows by it, and a metadata-only + * change here would re-export unchanged ring data. (This is also review item 10's rule + * applied proactively to this path.) + */ +internal fun clearPhoneStepsOnRow(existing: ActivityDailyEntity?): ActivityDailyEntity? = + if (existing == null || existing.phoneSteps == null) null + else existing.copy(phoneSteps = null) + /** * Is this reading one the ring will hand back from its own history, so that the two copies have to * be reconciled later ([EventPersistenceSubscriber.adoptRingsCopy])? Both halves are required: diff --git a/app/src/test/java/com/pulseloop/service/EventPersistencePhoneWriteTest.kt b/app/src/test/java/com/pulseloop/service/EventPersistencePhoneWriteTest.kt index b329187c..a16e7ed4 100644 --- a/app/src/test/java/com/pulseloop/service/EventPersistencePhoneWriteTest.kt +++ b/app/src/test/java/com/pulseloop/service/EventPersistencePhoneWriteTest.kt @@ -13,11 +13,15 @@ import org.junit.Test * distance. [phoneStepRow] is the pure decision this rests on (same pattern as * `EventPersistenceIdentityTest`'s helpers); the Room call around it is one line. * - * The four rules asserted here: + * The four [phoneStepRow] rules asserted here: * - only `phoneSteps`/`syncedAt`/`updatedAt` change on an existing row * - a brand-new row is created with `source = "phone"` * - writing the same value twice is a no-op * - `phoneSteps = 0` is a real value distinct from null + * + * Plus [clearPhoneStepsOnRow]'s contract (review item 4): + * - the returned row differs only in `phoneSteps = null` — no `updatedAt` bump (see item 10) + * - the call is a no-op when `phoneSteps` is already null, or when the row is absent */ class EventPersistencePhoneWriteTest { @@ -94,4 +98,41 @@ class EventPersistencePhoneWriteTest { // "explicit zero" with "no value": only equal-non-null values are the no-op case. assertNotNull(phoneStepRow(ringRow(phoneSteps = 0), dayStart, steps = 1)) } + + // ── clearPhoneStepsOnRow (review item 4) ───────────────────────────────── + + @Test + fun clearPhoneStepsNullsTheColumnWithoutTouchingAnythingElse() { + val existing = ringRow(phoneSteps = 8_200, source = "ring").copy( + activeMinutes = 42, syncedAt = 500L, updatedAt = 500L, + ) + val cleared = clearPhoneStepsOnRow(existing)!! + + assertNull(cleared.phoneSteps) + // Ring data columns untouched: + assertEquals(5_000, cleared.steps) + assertEquals(210.5, cleared.calories, 0.0) + assertEquals(4_321.0, cleared.distanceMeters, 0.0) + assertEquals("ring", cleared.source) + assertEquals(42, cleared.activeMinutes) + // Metadata untouched too: syncedAt is preserved, and updatedAt deliberately is + // — ActivityExporter selects on it, and a metadata-only change would re-export + // unchanged ring data (item 10's rule). + assertEquals(500L, cleared.syncedAt) + assertEquals(500L, cleared.updatedAt) + } + + @Test + fun clearPhoneStepsIsANoOpWhenAlreadyNull() { + // Every failed foreground fires PhoneStepSourceUnavailable. Only the first one + // (while phoneSteps was non-null) should write; the rest must be no-ops. + assertNull(clearPhoneStepsOnRow(ringRow(phoneSteps = null))) + } + + @Test + fun clearPhoneStepsIsANoOpWhenRowIsAbsent() { + // No row for today: nothing to clear. The ring hasn't written and the phone hasn't + // either, so display falls back to the null-row default (0 / no data) already. + assertNull(clearPhoneStepsOnRow(null)) + } } From dd45f241ff6e295be938ae67aafdcfeb42a4c304 Mon Sep 17 00:00:00 2001 From: leona Date: Sun, 25 Oct 2026 21:27:45 +0100 Subject: [PATCH 25/33] Phone write: preserve updatedAt on existing rows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review item 10. ActivityExporter selects rows by updatedAt, so every phoneSteps write re-selected the row and re-exported unchanged ring steps, calories and distance to Health Connect — data the ring had already put there and which hadn't moved. phoneStepRow now leaves updatedAt as the ring last wrote it on an existing row. syncedAt still moves (metadata about when the phone last re-confirmed the value). A brand-new phone-first row still stamps updatedAt = now, because that row didn't exist before and the exporter has to pick it up. Two new tests cover the contract: a first-write-on-existing-row case and a changed-value-on-existing-row case, both asserting updatedAt survives. --- .../service/EventPersistenceSubscriber.kt | 46 ++++++++++++------- .../service/EventPersistencePhoneWriteTest.kt | 27 +++++++++-- 2 files changed, 52 insertions(+), 21 deletions(-) diff --git a/app/src/main/java/com/pulseloop/service/EventPersistenceSubscriber.kt b/app/src/main/java/com/pulseloop/service/EventPersistenceSubscriber.kt index 8bf99cda..ab95271f 100644 --- a/app/src/main/java/com/pulseloop/service/EventPersistenceSubscriber.kt +++ b/app/src/main/java/com/pulseloop/service/EventPersistenceSubscriber.kt @@ -556,9 +556,11 @@ class EventPersistenceSubscriber( * * Under the separate-column design ([ActivityDailyEntity.phoneSteps]), an existing row's * **data columns** are untouched except for `phoneSteps`: `steps`, `distanceMeters`, - * `calories`, and `source` are left exactly as the ring wrote them. (`syncedAt` and - * `updatedAt` also move — metadata, not data; `updatedAt` is what ActivityExporter selects - * on.) The display picks between `steps` and `phoneSteps` at read time based on the user's + * `calories`, and `source` are left exactly as the ring wrote them. `syncedAt` also moves — + * metadata, not data. `updatedAt` is *preserved* (review item 10): it is what + * [com.pulseloop.health.exporters.ActivityExporter] selects on, so bumping it for a + * `phoneSteps`-only change would re-select the row and re-export unchanged ring data. The + * display picks between `steps` and `phoneSteps` at read time based on the user's * step-source preference, so switching source is a read-time choice, not a destructive * write: the ring's totals are never clobbered and switching back is instant. * @@ -570,15 +572,15 @@ class EventPersistenceSubscriber( * used to land in `steps`, the exported column; under the separate-column design the * self-feedback loop is broken by construction). A phone-first day still produces no HC * record: it carries `steps = 0`, and `isPlausibleSteps` floors at 1, so the plausibility - * guards skip it regardless of source. + * guards skip it regardless of source. A phone-first row also stamps `updatedAt = now` + * (see [phoneStepRow]) so the exporter's watermark query picks it up. * * Later ring writes preserve that source — see [upsertActivityDaily]. The old "overwrite * source and reset it back to ring on next ring write" dance is gone; nothing needs it any * more. * * A no-op when the day already holds this exact phone value: on every foreground the phone - * reader re-publishes today's total, and without the guard every pass would touch `updatedAt` - * (which ActivityExporter selects by) and re-export the same HC record. + * reader re-publishes today's total, so a value that hasn't moved must not produce a write. */ private suspend fun upsertActivityDailyFromPhone(ts: Long, steps: Int) { val dayStart = com.pulseloop.util.TimeUtil.startOfDayLocal(ts) @@ -907,14 +909,16 @@ class EventPersistenceSubscriber( * pattern as the sleep-merge helpers below). The contract, all of which is asserted in * `EventPersistencePhoneWriteTest`: * - * - **Only three columns change on an existing row:** `phoneSteps`, `syncedAt`, `updatedAt`. - * `steps`, `calories`, `distanceMeters`, and `source` are carried through untouched — that is - * what makes switching step source reversible. - * - **A brand-new row is created with `source = "phone"`**, so the day is visible in the "real" - * DAO queries even before the ring has data for it. + * - **Only two columns change on an existing row:** `phoneSteps` and `syncedAt`. `steps`, + * `calories`, `distanceMeters`, `source`, and `updatedAt` are carried through untouched — + * the first four keep the source switch reversible, and preserving `updatedAt` stops + * [com.pulseloop.health.exporters.ActivityExporter] from re-selecting the row for unchanged + * ring data (review item 10). + * - **A brand-new row is created with `source = "phone"`**, so the day is visible in the + * "real" DAO queries even before the ring has data for it. New rows also stamp + * `updatedAt = now` so a phone-first day is picked up by the exporter's watermark query. * - **Writing the same value twice is a no-op** (returns null): the phone reader re-publishes - * today's total on every foreground, and without this guard each pass would bump `updatedAt` - * — which `ActivityExporter` selects by — and re-export the same record. + * today's total on every foreground, so a value that hasn't moved must not produce a write. * - **`phoneSteps = 0` is a real value, distinct from null.** `null → 0` writes (the midnight * gap: the phone has begun counting and counted zero); `0 → 0` is a no-op. */ @@ -923,13 +927,23 @@ internal fun phoneStepRow( dayStart: Long, steps: Int, now: Long = System.currentTimeMillis(), -): ActivityDailyEntity? = - if (existing != null && existing.phoneSteps == steps) null - else (existing ?: ActivityDailyEntity(date = dayStart, source = StepSourcePrefs.SOURCE_PHONE)).copy( +): ActivityDailyEntity? = when { + // No-op when the day already holds this exact phone value. + existing != null && existing.phoneSteps == steps -> null + // Existing row: phoneSteps and syncedAt only. updatedAt is preserved — ActivityExporter + // selects on it, so a metadata-only phone write must not re-select the row and re-export + // unchanged ring steps/calories/distance (review item 10). + existing != null -> existing.copy(phoneSteps = steps, syncedAt = now) + // New phone-first row: updatedAt stamps "this row just appeared" so the exporter picks + // it up. + else -> ActivityDailyEntity( + date = dayStart, + source = StepSourcePrefs.SOURCE_PHONE, phoneSteps = steps, syncedAt = now, updatedAt = now, ) +} /** * The row a [PulseEvent.PhoneStepSourceUnavailable] should write for today — or null when it diff --git a/app/src/test/java/com/pulseloop/service/EventPersistencePhoneWriteTest.kt b/app/src/test/java/com/pulseloop/service/EventPersistencePhoneWriteTest.kt index a16e7ed4..bd0fb322 100644 --- a/app/src/test/java/com/pulseloop/service/EventPersistencePhoneWriteTest.kt +++ b/app/src/test/java/com/pulseloop/service/EventPersistencePhoneWriteTest.kt @@ -14,7 +14,8 @@ import org.junit.Test * `EventPersistenceIdentityTest`'s helpers); the Room call around it is one line. * * The four [phoneStepRow] rules asserted here: - * - only `phoneSteps`/`syncedAt`/`updatedAt` change on an existing row + * - only `phoneSteps`/`syncedAt` change on an existing row — `updatedAt` is preserved so the + * exporter does not re-select the row for unchanged ring data (review item 10) * - a brand-new row is created with `source = "phone"` * - writing the same value twice is a no-op * - `phoneSteps = 0` is a real value distinct from null @@ -43,13 +44,13 @@ class EventPersistencePhoneWriteTest { ) @Test - fun phoneWriteOnlyTouchesPhoneStepsSyncedAtAndUpdatedAt() { - val existing = ringRow() + fun phoneWriteTouchesPhoneStepsAndSyncedAtOnly() { + val existing = ringRow().copy(updatedAt = 500L) val written = phoneStepRow(existing, dayStart, steps = 8_200, now = 999L)!! assertEquals(8_200, written.phoneSteps) - assertEquals(999L, written.syncedAt) - assertEquals(999L, written.updatedAt) + assertEquals(999L, written.syncedAt) // syncedAt bumps + assertEquals(500L, written.updatedAt) // preserved — review item 10 // Ring's own columns, untouched: assertEquals(5_000, written.steps) @@ -58,6 +59,20 @@ class EventPersistencePhoneWriteTest { assertEquals("ring", written.source) } + @Test + fun changedPhoneValueAlsoPreservesUpdatedAt() { + // The realistic case: an existing day has phoneSteps = 8_200, the user walks 300 + // more, refresh() publishes 8_500. phoneSteps and syncedAt move; updatedAt must not, + // or ActivityExporter re-selects the row and re-exports unchanged ring steps/ + // calories/distance (review item 10). + val existing = ringRow(phoneSteps = 8_200).copy(updatedAt = 500L) + val written = phoneStepRow(existing, dayStart, steps = 8_500, now = 999L)!! + + assertEquals(8_500, written.phoneSteps) + assertEquals(999L, written.syncedAt) + assertEquals(500L, written.updatedAt) + } + @Test fun firstPhoneWriteOnADayCreatesRowWithPhoneSource() { val written = phoneStepRow(null, dayStart = dayStart, steps = 8_200, now = 999L)!! @@ -65,6 +80,8 @@ class EventPersistencePhoneWriteTest { assertEquals(dayStart, written.date) assertEquals(8_200, written.phoneSteps) assertEquals("phone", written.source) + assertEquals(999L, written.syncedAt) + assertEquals(999L, written.updatedAt) // new rows stamp updatedAt so the exporter picks them up assertEquals(0, written.steps) // default — nothing has written the ring column yet assertEquals(0.0, written.calories, 0.0) // default assertEquals(0.0, written.distanceMeters, 0.0) From e21d481ea6c7f60099be0fe5ee1642b3e5e3cf9b Mon Sep 17 00:00:00 2001 From: leona Date: Sun, 25 Oct 2026 21:57:03 +0100 Subject: [PATCH 26/33] Coach: read display steps through DisplaySteps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review item 5, part 1 of 3. The coach could quote ring steps (9,800) while Today showed phone steps (8,200) in phone mode, because CoachContextBuilder and CoachSummaryContextBuilder read the raw steps column and never consulted the step-source preference. Introduces com.pulseloop.data.DisplaySteps as the one place the preference is applied to display values, and threads preferPhone through the coach context and summary builders. Same step-source-resolved count the Today tile shows, so the two cannot disagree. DisplaySteps is also the entry point for parts 2 and 3: the remaining readers (MetricsService, coach tools) and the already-correct call sites (ViewModels, widget, notifications) will migrate to it in follow-up commits. The exporter and archive deliberately do not — they must serialize the ring's actual column. --- .../main/java/com/pulseloop/AppServices.kt | 3 +- .../coach/context/CoachContextBuilder.kt | 8 ++- .../summaries/CoachSummaryContextBuilder.kt | 28 +++++---- .../summaries/CoachSummaryCoordinator.kt | 7 ++- .../coach/summaries/CoachSummaryService.kt | 8 ++- .../java/com/pulseloop/data/DisplaySteps.kt | 58 +++++++++++++++++++ .../java/com/pulseloop/ui/PulseLoopApp.kt | 1 + .../com/pulseloop/ui/viewmodels/ViewModels.kt | 12 +++- 8 files changed, 107 insertions(+), 18 deletions(-) create mode 100644 app/src/main/java/com/pulseloop/data/DisplaySteps.kt diff --git a/app/src/main/java/com/pulseloop/AppServices.kt b/app/src/main/java/com/pulseloop/AppServices.kt index 6c740795..c10a75e4 100644 --- a/app/src/main/java/com/pulseloop/AppServices.kt +++ b/app/src/main/java/com/pulseloop/AppServices.kt @@ -35,6 +35,7 @@ class AppServices private constructor(context: Context) { val db: PulseLoopDatabase = PulseLoopDatabase.getInstance(context) val bleClient = RingBLEClient(context) val apiKeyStore = ApiKeyStore(context) + val stepSourcePrefs = com.pulseloop.settings.StepSourcePrefs(context) val coordinator = RingSyncCoordinator(bleClient, db, apiKeyStore) val gpsRecorder = GpsRouteRecorder(context, db) val liveWorkout = LiveWorkoutManager(coordinator, db, gpsRecorder, context) @@ -47,7 +48,7 @@ class AppServices private constructor(context: Context) { ) val batteryAlerts = BatteryAlertMonitor(context) val providerStore = CoachProviderSettingsStore(context) - val summaryCoordinator = CoachSummaryCoordinator(db, apiKeyStore, providerStore) + val summaryCoordinator = CoachSummaryCoordinator(db, apiKeyStore, providerStore, stepSourcePrefs) // iOS #94: runs the due check-in slot the moment a full sync completes, so a slot the periodic // worker skipped for stale data is delivered a few minutes later instead of being lost. diff --git a/app/src/main/java/com/pulseloop/coach/context/CoachContextBuilder.kt b/app/src/main/java/com/pulseloop/coach/context/CoachContextBuilder.kt index 51959a6b..b214be93 100644 --- a/app/src/main/java/com/pulseloop/coach/context/CoachContextBuilder.kt +++ b/app/src/main/java/com/pulseloop/coach/context/CoachContextBuilder.kt @@ -17,6 +17,7 @@ object CoachContextBuilder { suspend fun build( db: PulseLoopDatabase, + preferPhone: Boolean, conversationSummary: String? = null, environment: EnvironmentContext? = null, ): CoachContextPacket { @@ -39,8 +40,11 @@ object CoachContextBuilder { val sevenDaysAgo = todayStart - 7 * 24 * 3600_000L val activityWeek = (if (activityHasReal) db.activityDailyDao().recentReal(7) else db.activityDailyDao().recent(7)).filter { it.date >= sevenDaysAgo } - val stepsWeek = activityWeek.map { it.steps.toDouble() } - val daysAvailable = activityWeek.count { it.steps > 0 } + // Display steps: ring mode reads `steps`, phone mode reads `phoneSteps` when present + // (review item 5, PR #98). Same figure the Today tile shows, so the coach cannot quote + // 9,800 while Today reads 8,200. Falls back to the ring's value on an un-backfilled day. + val stepsWeek = com.pulseloop.data.DisplaySteps.counts(activityWeek, preferPhone).map { it.toDouble() } + val daysAvailable = com.pulseloop.data.DisplaySteps.counts(activityWeek, preferPhone).count { it > 0 } // Resting HR: average of HR values in the last 24h while at rest (approximated as lowest 25th percentile) val hr24h = if (db.measurementDao().hasReal(MeasurementKind.HEART_RATE.name)) diff --git a/app/src/main/java/com/pulseloop/coach/summaries/CoachSummaryContextBuilder.kt b/app/src/main/java/com/pulseloop/coach/summaries/CoachSummaryContextBuilder.kt index bb8974c7..363bd5ba 100644 --- a/app/src/main/java/com/pulseloop/coach/summaries/CoachSummaryContextBuilder.kt +++ b/app/src/main/java/com/pulseloop/coach/summaries/CoachSummaryContextBuilder.kt @@ -29,8 +29,8 @@ object CoachSummaryContextBuilder { // ── Today ──────────────────────────────────────────────────────────── - suspend fun today(db: PulseLoopDatabase): Built { - val packet = CoachContextBuilder.build(db) + suspend fun today(db: PulseLoopDatabase, preferPhone: Boolean): Built { + val packet = CoachContextBuilder.build(db, preferPhone) val todayDate = packet.today @Serializable @@ -53,7 +53,7 @@ object CoachSummaryContextBuilder { val tp = TodayPacket( today = todayDate, - steps = activity?.steps, + steps = com.pulseloop.data.DisplaySteps.count(activity, preferPhone), calories = activity?.calories, distanceKm = activity?.distanceMeters?.let { it / 1000 }, activeMinutes = activity?.activeMinutes, @@ -63,17 +63,21 @@ object CoachSummaryContextBuilder { ) val contextJson = json.encodeToString(tp) + // The signature must include the *display* step figure, not the raw column — a phone-mode + // change that leaves `steps` untouched but moves `phoneSteps` has to trigger a regeneration + // or the card would keep quoting the old value (review item 5, PR #98). val sig = signature(listOf( - activity?.steps?.toString(), activity?.calories?.toInt()?.toString(), + tp.steps?.toString(), activity?.calories?.toInt()?.toString(), activity?.activeMinutes?.toString(), tp.distanceKm?.toString(), latestHr?.toInt()?.toString(), latestSpo2?.toInt()?.toString(), )) - // Scripted fallback - val title = if (activity != null && activity.steps > 0) "Today's snapshot" else "No activity synced yet" - val body = if (activity != null && activity.steps > 0) - "${activity.steps} steps so far today. Your ring is tracking — sync for the latest readings." - else "I don't have today's activity from the ring yet. Wear your ring and sync for a daily recap." + // Scripted fallback — same display value as the Today tile, so the two cannot disagree. + val displaySteps = tp.steps ?: 0 + val title = if (displaySteps > 0) "Today's snapshot" else "No activity synced yet" + val body = if (displaySteps > 0) + "$displaySteps steps so far today. Your ring is tracking — sync for the latest readings." + else "I don't have today's activity yet. Wear your ring and sync for a daily recap." return Built( scopeKey = todayDate, @@ -85,7 +89,7 @@ object CoachSummaryContextBuilder { // ── Sleep Day ──────────────────────────────────────────────────────── - suspend fun sleepDay(db: PulseLoopDatabase): Built? { + suspend fun sleepDay(db: PulseLoopDatabase, preferPhone: Boolean): Built? { val todayStart = java.time.LocalDate.now().atStartOfDay(java.time.ZoneId.systemDefault()) .toInstant().toEpochMilli() val session = db.sleepSessionDao().byDay(todayStart) ?: return null @@ -93,7 +97,9 @@ object CoachSummaryContextBuilder { val blocks = db.sleepStageBlockDao().forSession(session.id) val score = SleepScore.calculate(session, blocks) - val activitySteps = db.activityDailyDao().byDay(todayStart)?.steps + val activitySteps = com.pulseloop.data.DisplaySteps.count( + db.activityDailyDao().byDay(todayStart), preferPhone, + ) @Serializable data class SleepDayPacket( diff --git a/app/src/main/java/com/pulseloop/coach/summaries/CoachSummaryCoordinator.kt b/app/src/main/java/com/pulseloop/coach/summaries/CoachSummaryCoordinator.kt index d8a189d9..825c70fc 100644 --- a/app/src/main/java/com/pulseloop/coach/summaries/CoachSummaryCoordinator.kt +++ b/app/src/main/java/com/pulseloop/coach/summaries/CoachSummaryCoordinator.kt @@ -19,8 +19,13 @@ class CoachSummaryCoordinator( /** Optional multi-provider settings, threaded through to the service so * summaries follow the selected coach provider. */ providerSettings: com.pulseloop.coach.config.CoachProviderSettingsStore? = null, + /** The user's step-source preference, threaded to the service so the coach's + * today/sleep summaries quote the same step figure Today does (review item 5, + * PR #98). The service reads it per generation, so a toggle takes effect on + * the next card regeneration without an app restart. */ + stepSourcePrefs: com.pulseloop.settings.StepSourcePrefs, ) { - private val service = CoachSummaryService(db, apiKeyStore, providerSettings) + private val service = CoachSummaryService(db, apiKeyStore, providerSettings, stepSourcePrefs) private val scope = CoroutineScope(Dispatchers.Main + SupervisorJob()) private var streamJob: Job? = null private var debounceJob: Job? = null diff --git a/app/src/main/java/com/pulseloop/coach/summaries/CoachSummaryService.kt b/app/src/main/java/com/pulseloop/coach/summaries/CoachSummaryService.kt index 5fb171ef..d555b73a 100644 --- a/app/src/main/java/com/pulseloop/coach/summaries/CoachSummaryService.kt +++ b/app/src/main/java/com/pulseloop/coach/summaries/CoachSummaryService.kt @@ -26,6 +26,10 @@ class CoachSummaryService( * `CoachClientResolver` (Gemini/OpenRouter follow the provider picker). * null keeps the legacy OpenAI-only behavior. */ private val providerSettings: CoachProviderSettingsStore? = null, + /** The user's step-source preference (review item 5, PR #98). Applied per regeneration so + * the coach's today/sleep summaries quote the same steps Today shows — Path B's separate + * columns otherwise let the coach read ring steps while Today read phone steps. */ + private val stepSourcePrefs: com.pulseloop.settings.StepSourcePrefs, ) { /** Minimum gap between Today / aggregate-sleep regenerations. */ private val minIntervalMs = 2 * 3600_000L // 2 hours @@ -42,7 +46,7 @@ class CoachSummaryService( // ── Refresh (self-gating) ──────────────────────────────────────────── suspend fun refreshTodayIfNeeded() { - val built = CoachSummaryContextBuilder.today(db) + val built = CoachSummaryContextBuilder.today(db, stepSourcePrefs.preferPhone()) val existing = db.coachSummaryDao().get(CoachSummaryKind.TODAY.rawValue, built.scopeKey) val now = System.currentTimeMillis() @@ -54,7 +58,7 @@ class CoachSummaryService( } suspend fun refreshSleepDayIfNeeded(now: Long = System.currentTimeMillis()) { - val built = CoachSummaryContextBuilder.sleepDay(db) ?: return + val built = CoachSummaryContextBuilder.sleepDay(db, stepSourcePrefs.preferPhone()) ?: return val endAt = built.sleepSessionEnd ?: return val lastFullSyncAt = db.deviceDao().current()?.lastFullSyncAt if (!CoachSleepSyncGate.sleepDaySafeToSummarize(endAt, lastFullSyncAt, now)) return diff --git a/app/src/main/java/com/pulseloop/data/DisplaySteps.kt b/app/src/main/java/com/pulseloop/data/DisplaySteps.kt new file mode 100644 index 00000000..7cf4f9e9 --- /dev/null +++ b/app/src/main/java/com/pulseloop/data/DisplaySteps.kt @@ -0,0 +1,58 @@ +package com.pulseloop.data + +import com.pulseloop.data.entity.ActivityDailyEntity + +/** + * The one place the step-source preference is applied to display values. + * + * Path B keeps phone and ring steps in separate columns on [ActivityDailyEntity] + * (`phoneSteps` vs `steps`). Every screen, the widgets, notifications, the coach + * context and the coach tools must pick between them the same way, from the + * user's "Ring" / "Phone" preference. Doing that at each call site meant fifteen + * places had to agree — and the coach disagreed with Today for two weeks + * (review item 5, PR #98). + * + * This object is that place. Callers pass `preferPhone: Boolean` — resolved once + * at the boundary (a ViewModel's `preferPhone()` lambda, `AppServices`' single + * `StepSourcePrefs`), threaded down as a plain parameter. It does not read + * SharedPreferences itself, so callers can control when the preference is + * consulted (per emission, per composition) and tests can drive it directly. + * + * **Not for the exporter or the archive.** Those must serialize the ring's + * actual `steps` column, not whatever the user has chosen to display. Passing + * a `preferPhone` value into an export path would silently write phone steps + * into the ring's column on the next restore. + */ +object DisplaySteps { + + /** + * The step count a screen should show for [row] under the user's preference. + * + * Null-safe (a missing row has no count). Phone mode returns [ActivityDailyEntity.phoneSteps] + * when it has one, else the ring's [ActivityDailyEntity.steps]; ring mode always returns + * `steps`. That fallback is what makes an un-backfilled day still show something in Phone + * mode instead of a spurious zero. + */ + fun count(row: ActivityDailyEntity?, preferPhone: Boolean): Int? = + row?.displaySteps(preferPhone) + + /** + * The step counts for [rows], in the same order, under the user's preference. + * Vector form of [count] — used by the 7-day hero series and week widgets. + */ + fun counts(rows: List, preferPhone: Boolean): List = + rows.map { it.displaySteps(preferPhone) } + + /** + * [row] with its `steps` column projected to the display value, for callers + * that hold a whole entity in their state (`ActivityViewModel.recentDays`, + * the widget snapshot). Only `steps` is rewritten; `distanceMeters`, + * `calories` and the rest of the row stay as the ring wrote them. + */ + fun project(row: ActivityDailyEntity?, preferPhone: Boolean): ActivityDailyEntity? = + row?.forDisplay(preferPhone) + + /** [rows] projected the same way, same order. */ + fun project(rows: List, preferPhone: Boolean): List = + rows.map { it.forDisplay(preferPhone) } +} \ No newline at end of file diff --git a/app/src/main/java/com/pulseloop/ui/PulseLoopApp.kt b/app/src/main/java/com/pulseloop/ui/PulseLoopApp.kt index 244b91e2..f8034211 100644 --- a/app/src/main/java/com/pulseloop/ui/PulseLoopApp.kt +++ b/app/src/main/java/com/pulseloop/ui/PulseLoopApp.kt @@ -142,6 +142,7 @@ fun PulseLoopApp() { val coachVM = remember { CoachViewModel( db, coachOrchestrator, + preferPhone = { stepSourcePrefs.preferPhone() }, attachmentPayloads = { refs -> com.pulseloop.coach.attachments.CoachAttachmentStore.payloads(context, refs) }, diff --git a/app/src/main/java/com/pulseloop/ui/viewmodels/ViewModels.kt b/app/src/main/java/com/pulseloop/ui/viewmodels/ViewModels.kt index 331a96b8..79ac2818 100644 --- a/app/src/main/java/com/pulseloop/ui/viewmodels/ViewModels.kt +++ b/app/src/main/java/com/pulseloop/ui/viewmodels/ViewModels.kt @@ -930,6 +930,12 @@ fun VitalsViewModel.VitalsState.toCardInputs(units: UnitSystem): VitalsCardFacto class CoachViewModel( private val db: PulseLoopDatabase, private val orchestrator: com.pulseloop.coach.orchestration.CoachOrchestrator, + /** + * Resolves the current step-source preference (review item 5, PR #98). Injected as a lambda + * rather than a `StepSourcePrefs` so the VM stays context-free like [attachmentPayloads]; + * the lambda is called per turn so a toggle made mid-session is reflected on the next turn. + */ + private val preferPhone: () -> Boolean = { false }, /** Resolves staged attachment refs to wire payloads (injected so the VM stays context-free). */ private val attachmentPayloads: (List) -> List = { emptyList() }, /** Opt-in city + weather (iOS #65d), injected so the VM stays context-free like [attachmentPayloads]. */ @@ -1099,7 +1105,11 @@ class CoachViewModel( attachmentsJson = com.pulseloop.coach.attachments.CoachAttachmentRef.encode(attachments), )) try { - val packet = com.pulseloop.coach.context.CoachContextBuilder.build(db, environment = environmentSnapshot()) + val packet = com.pulseloop.coach.context.CoachContextBuilder.build( + db, + preferPhone = preferPhone(), + environment = environmentSnapshot(), + ) // Drop error bubbles: they're app-generated diagnostics (a provider HTTP error), // not real assistant turns. Replaying them as assistant history would feed the // model garbage like "Coach error · HTTP 404 …" as if it had said it. From c38f2b66a0b3fd1cbe0def5e5f7bdf6e6b0776c7 Mon Sep 17 00:00:00 2001 From: leona Date: Sun, 25 Oct 2026 22:08:34 +0100 Subject: [PATCH 27/33] Coach tools: thread step-source preference into ToolExecutionContext MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review item 5, part 2 of 3. Preparation only — no behavior change yet. The retrieval tools that quote steps (get_daily_summary, get_range_summary, get_goal_progress, get_recent_anomalies) still read the raw steps column; wiring them to DisplaySteps is part 3. Adds preferPhone to ToolExecutionContext, resolved per turn from AppServices.stepSourcePrefs at the tool context factory so a toggle made mid-session lands on the next call. Also adds preferPhone to MetricsService.buildTodaySummary — the function has no callers today, but the reviewer named it, and if a future caller wires it up it should give the same answer Today does. MetricsService is now honest: steps = DisplaySteps.count(row, preferPhone). --- .../java/com/pulseloop/coach/tools/CoachTool.kt | 13 +++++++++++++ .../java/com/pulseloop/service/MetricsService.kt | 13 +++++++++++-- app/src/main/java/com/pulseloop/ui/PulseLoopApp.kt | 3 +++ 3 files changed, 27 insertions(+), 2 deletions(-) diff --git a/app/src/main/java/com/pulseloop/coach/tools/CoachTool.kt b/app/src/main/java/com/pulseloop/coach/tools/CoachTool.kt index 826bfc4b..249c27b1 100644 --- a/app/src/main/java/com/pulseloop/coach/tools/CoachTool.kt +++ b/app/src/main/java/com/pulseloop/coach/tools/CoachTool.kt @@ -42,6 +42,19 @@ data class ToolExecutionContext( * compile unchanged (iOS's ToolExecutionContext has the same optional foodClient). */ val foodClient: com.pulseloop.nutrition.FoodDatabaseClient? = null, + /** + * The resolved step-source preference (review item 5, PR #98). Path B keeps phone and + * ring steps in separate columns; the retrieval tools must quote the same figure the + * Today tile shows. Populated per turn from + * [com.pulseloop.AppServices.stepSourcePrefs] at the tool context factory, so a toggle + * made mid-session is reflected on the next call. + * + * Defaults to false (ring steps) so existing test harnesses compile unchanged. Any + * production call site that reads `.steps` from an `activity_daily` row and is + * user-visible must pass this through `com.pulseloop.data.DisplaySteps` — see + * [com.pulseloop.data.DisplaySteps] for the pick rule. + */ + val preferPhone: Boolean = false, ) data class CoachFeatureFlags( diff --git a/app/src/main/java/com/pulseloop/service/MetricsService.kt b/app/src/main/java/com/pulseloop/service/MetricsService.kt index fde35045..903ea187 100644 --- a/app/src/main/java/com/pulseloop/service/MetricsService.kt +++ b/app/src/main/java/com/pulseloop/service/MetricsService.kt @@ -33,7 +33,16 @@ object MetricsService { val spo2Trend24h: List = emptyList(), ) - suspend fun buildTodaySummary(db: PulseLoopDatabase): TodaySummary { + /** + * The day's activity totals for display. + * + * [preferPhone] is the resolved step-source preference (review item 5, PR #98). Pass it + * through [com.pulseloop.data.DisplaySteps] to pick between the ring's `steps` column and + * the phone's `phoneSteps` column per Path B. No callers today — kept for parity with the + * iOS reference and so that any future wiring of this aggregation gets the same answer + * Today does. + */ + suspend fun buildTodaySummary(db: PulseLoopDatabase, preferPhone: Boolean): TodaySummary { val now = System.currentTimeMillis() val todayStart = Instant.ofEpochMilli(now).atZone(java.time.ZoneId.systemDefault()) .truncatedTo(ChronoUnit.DAYS).toInstant().toEpochMilli() @@ -61,7 +70,7 @@ object MetricsService { val restingHr = hrValues.filter { it <= 72.0 }.minOrNull() return TodaySummary( - steps = todayActivity?.steps, + steps = com.pulseloop.data.DisplaySteps.count(todayActivity, preferPhone), calories = todayActivity?.calories, distanceMeters = todayActivity?.distanceMeters, activeMinutes = todayActivity?.activeMinutes, diff --git a/app/src/main/java/com/pulseloop/ui/PulseLoopApp.kt b/app/src/main/java/com/pulseloop/ui/PulseLoopApp.kt index f8034211..3bd4272d 100644 --- a/app/src/main/java/com/pulseloop/ui/PulseLoopApp.kt +++ b/app/src/main/java/com/pulseloop/ui/PulseLoopApp.kt @@ -120,6 +120,9 @@ fun PulseLoopApp() { flags = flags, coordinator = coordinator, foodClient = foodClient, + // Review item 5, part 2 of 3 — resolved per turn so a toggle made + // mid-session is reflected on the next tool call. + preferPhone = services.stepSourcePrefs.preferPhone(), ) }, ) From 46bca8064898a5cb2fc1ed23ab2b7cf6e690555f Mon Sep 17 00:00:00 2001 From: leona Date: Sun, 25 Oct 2026 22:19:25 +0100 Subject: [PATCH 28/33] Coach tools: read display steps through DisplaySteps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review item 5, part 3 of 3. The retrieval tools that quote steps — get_daily_summary, get_range_summary, get_goal_progress, get_recent_anomalies — and the metric-series path (get_metric_series -> CoachDataAccess.seriesPoints -> dailySeries) all read the raw steps column. In phone mode they now quote the phone's step count, same as the Today tile. Threads preferPhone through CoachDataAccess.dailySeries and seriesPoints, and every retrieval tool that reads .steps now goes through DisplaySteps with ctx.preferPhone (populated per turn in the previous commit). Same behavior neutral for Ring mode — DisplaySteps falls back to the ring column — and now consistent in Phone mode: the coach quotes what the user sees. --- .../com/pulseloop/coach/tools/CoachDataAccess.kt | 14 ++++++++------ .../pulseloop/coach/tools/ToolImplementations.kt | 13 ++++++++----- 2 files changed, 16 insertions(+), 11 deletions(-) diff --git a/app/src/main/java/com/pulseloop/coach/tools/CoachDataAccess.kt b/app/src/main/java/com/pulseloop/coach/tools/CoachDataAccess.kt index b3be595f..89f0491d 100644 --- a/app/src/main/java/com/pulseloop/coach/tools/CoachDataAccess.kt +++ b/app/src/main/java/com/pulseloop/coach/tools/CoachDataAccess.kt @@ -129,13 +129,14 @@ object CoachDataAccess { // ── Daily series ────────────────────────────────────────────────────── suspend fun dailySeries( - db: PulseLoopDatabase, metric: String, start: String, end: String + db: PulseLoopDatabase, metric: String, start: String, end: String, + preferPhone: Boolean, ): List> { return when (metric) { "steps", "calories", "distance", "active_minutes" -> { activityRows(db, start, end).map { row -> val v = when (metric) { - "steps" -> row.steps.toDouble() + "steps" -> com.pulseloop.data.DisplaySteps.count(row, preferPhone)?.toDouble() ?: 0.0 "calories" -> row.calories "distance" -> (row.distanceMeters / 1000.0).roundTo(2) "active_minutes" -> row.activeMinutes.toDouble() @@ -166,7 +167,8 @@ object CoachDataAccess { // ── Series points ───────────────────────────────────────────────────── suspend fun seriesPoints( - db: PulseLoopDatabase, metric: String, start: String, end: String, granularity: String + db: PulseLoopDatabase, metric: String, start: String, end: String, granularity: String, + preferPhone: Boolean, ): List> { val labeler: (Long) -> String = when (granularity) { "hour" -> { ts -> hourLabel(ts) } @@ -182,7 +184,7 @@ object CoachDataAccess { Instant.ofEpochMilli(it.timestamp).atZone(ZoneId.systemDefault()) .truncatedTo(ChronoUnit.HOURS).toInstant().toEpochMilli() }.map { (h, ms) -> h to ms.map { it.value }.average().roundTo(1) }.sortedBy { it.first } - "day" -> dailySeries(db, metric, start, end) + "day" -> dailySeries(db, metric, start, end, preferPhone) else -> rows.map { it.timestamp to it.value } } } @@ -193,11 +195,11 @@ object CoachDataAccess { Instant.ofEpochMilli(it.timestamp).atZone(ZoneId.systemDefault()) .truncatedTo(ChronoUnit.HOURS).toInstant().toEpochMilli() }.map { (h, ms) -> h to ms.map { it.value }.average().roundTo(1) }.sortedBy { it.first } - "day" -> dailySeries(db, metric, start, end) + "day" -> dailySeries(db, metric, start, end, preferPhone) else -> rows.map { it.timestamp to it.value } } } - else -> dailySeries(db, metric, start, end) + else -> dailySeries(db, metric, start, end, preferPhone) } return downsample(dated, 300).map { labeler(it.first) to it.second } diff --git a/app/src/main/java/com/pulseloop/coach/tools/ToolImplementations.kt b/app/src/main/java/com/pulseloop/coach/tools/ToolImplementations.kt index cb282b27..62a085d9 100644 --- a/app/src/main/java/com/pulseloop/coach/tools/ToolImplementations.kt +++ b/app/src/main/java/com/pulseloop/coach/tools/ToolImplementations.kt @@ -60,13 +60,14 @@ object RetrievalTools { val dayTs = CoachDataAccess.parseLocalDate(date) if (dayTs == null) return@runBlocking """{"error":"invalid date: $date"}""" val activity = db.activityDailyDao().byDay(dayTs) + val displaySteps = com.pulseloop.data.DisplaySteps.count(activity, ctx.preferPhone) val hrVals = db.measurementDao().range(MeasurementKind.HEART_RATE.name, dayTs, dayTs + 86400000L) val hrStats = if (hrVals.isNotEmpty()) { val vals = hrVals.map { it.value } """"mean":${(vals.average() * 10).toLong() / 10.0},"min":${vals.minOrNull()!!.toInt()},"max":${vals.maxOrNull()!!.toInt()}"""" } else null val sleep = db.sleepSessionDao().byDay(dayTs) - """{"date":"$date","data_available":${activity != null || hrVals.isNotEmpty()},"activity":{"${if (activity != null) """steps":${activity.steps},"calories":${(activity.calories * 10).toLong() / 10.0},"distance_km":${(activity.distanceMeters / 1000 * 10).toLong() / 10.0},"active_minutes":${activity.activeMinutes}""" else """steps":0,"calories":0,"distance_km":0,"active_minutes":0"""}},"hr":{${hrStats ?: """"mean":null,"min":null,"max":null""""}},"sleep":${if (sleep != null) """{"total_min":${sleep.totalMinutes},"score":${sleep.totalMinutes / 5}}""" else "null"}}""" + """{"date":"$date","data_available":${activity != null || hrVals.isNotEmpty()},"activity":{"${if (activity != null) """steps":${displaySteps ?: 0},"calories":${(activity.calories * 10).toLong() / 10.0},"distance_km":${(activity.distanceMeters / 1000 * 10).toLong() / 10.0},"active_minutes":${activity.activeMinutes}""" else """steps":0,"calories":0,"distance_km":0,"active_minutes":0"""}},"hr":{${hrStats ?: """"mean":null,"min":null,"max":null""""}},"sleep":${if (sleep != null) """{"total_min":${sleep.totalMinutes},"score":${sleep.totalMinutes / 5}}""" else "null"}}""" } ToolResult(result) } @@ -95,7 +96,7 @@ object RetrievalTools { val start = params["start_date"] ?: return@CoachToolDef ToolResult("""{"error":"missing 'start_date'"}""", isError = true) val end = params["end_date"] ?: start val result = kotlinx.coroutines.runBlocking { - val points = CoachDataAccess.seriesPoints(db, metric, start, end, "day") + val points = CoachDataAccess.seriesPoints(db, metric, start, end, "day", ctx.preferPhone) val sb = StringBuilder() sb.append("""{"metric":"$metric","points":[""") points.forEachIndexed { i, (date, value) -> @@ -178,7 +179,7 @@ object RetrievalTools { val d = java.time.Instant.ofEpochMilli(it.date).atZone(java.time.ZoneId.systemDefault()).toLocalDate().toString() d >= start && d <= end } - val totalSteps = rows.sumOf { it.steps } + val totalSteps = com.pulseloop.data.DisplaySteps.counts(rows, ctx.preferPhone).sum() sb.append(""""activity":{"days_available":${rows.size},"totals":{"steps":$totalSteps,"calories":${rows.sumOf { it.calories }},"active_minutes":${rows.sumOf { it.activeMinutes }}}},""") } val hrData = db.measurementDao().range(MeasurementKind.HEART_RATE.name, 0, System.currentTimeMillis()) @@ -326,7 +327,8 @@ object RetrievalTools { val goal = kotlinx.coroutines.runBlocking { db.userGoalDao().get() } val todayStart = java.time.Instant.now().atZone(java.time.ZoneId.systemDefault()).truncatedTo(java.time.temporal.ChronoUnit.DAYS).toInstant().toEpochMilli() val today = kotlinx.coroutines.runBlocking { db.activityDailyDao().byDay(todayStart) } - ToolResult("""{"today":{"steps":${today?.steps ?: 0},"steps_goal":${goal?.steps ?: 10000},"active_minutes":${today?.activeMinutes ?: 0},"active_minutes_goal":${goal?.activeMinutes ?: 45}},"sleep_hours_goal":${(goal?.sleepMinutes ?: 480) / 60}}""") + val displaySteps = com.pulseloop.data.DisplaySteps.count(today, ctx.preferPhone) ?: 0 + ToolResult("""{"today":{"steps":$displaySteps,"steps_goal":${goal?.steps ?: 10000},"active_minutes":${today?.activeMinutes ?: 0},"active_minutes_goal":${goal?.activeMinutes ?: 45}},"sleep_hours_goal":${(goal?.sleepMinutes ?: 480) / 60}}""") } // ── get_recent_anomalies ─────────────────────────────────────────── @@ -343,7 +345,8 @@ object RetrievalTools { )), ) { _, ctx -> val db = ctx.db ?: return@CoachToolDef ToolResult("""{"error":"database not available"}""", isError = true) - val recentSteps = kotlinx.coroutines.runBlocking { db.activityDailyDao().recent(14) }.map { it.steps.toDouble() } + val recentRows = kotlinx.coroutines.runBlocking { db.activityDailyDao().recent(14) } + val recentSteps = com.pulseloop.data.DisplaySteps.counts(recentRows, ctx.preferPhone).map { it.toDouble() } val stepOutliers = AnalysisEngine.outliers(recentSteps) ToolResult("""{"steps_outliers":${stepOutliers.size},"steps_analyzed":${recentSteps.size}}""") } From 3d9be012b7ee4155b5a1b64d580187519d8f913a Mon Sep 17 00:00:00 2001 From: leona Date: Mon, 26 Oct 2026 08:37:54 +0100 Subject: [PATCH 29/33] Display steps: route every reader through DisplaySteps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review item 5, part 4 of 4. Behavior-neutral — the four call sites here already used the right column (the entity methods did the same pick). The change is that they now go through DisplaySteps, and the entity methods become internal so nothing outside the data package can bypass it. That is the "one place" the reviewer asked for: fifteen call sites across screens, widgets, notifications, the coach context, and the tools all read the same object, and the entity methods can no longer be reached from any of them. New readers have a compile-time hint — call DisplaySteps. The activity exporter and DataArchiveService deliberately do not use it: they must serialize the ring's actual steps column, not the display value. DisplaySteps' KDoc calls that out. --- .../com/pulseloop/data/entity/CoreEntities.kt | 17 ++++++++++++----- .../notifications/NotificationContextBuilder.kt | 2 +- .../com/pulseloop/ui/viewmodels/ViewModels.kt | 16 +++++++++------- .../widgets/WidgetSnapshotPublisher.kt | 2 +- 4 files changed, 23 insertions(+), 14 deletions(-) diff --git a/app/src/main/java/com/pulseloop/data/entity/CoreEntities.kt b/app/src/main/java/com/pulseloop/data/entity/CoreEntities.kt index 62797814..f76b467d 100644 --- a/app/src/main/java/com/pulseloop/data/entity/CoreEntities.kt +++ b/app/src/main/java/com/pulseloop/data/entity/CoreEntities.kt @@ -111,19 +111,26 @@ data class ActivityDailyEntity( * (Path B read-time selection, PR #98). Ring mode shows the ring's own column; phone mode * shows [phoneSteps] when the phone has a value for the day, falling back to the ring's * column when it does not — so a day the phone never backfilled still shows something. + * + * `internal` on purpose (review item 5, PR #98). The one production caller is + * [com.pulseloop.data.DisplaySteps] — the one place the preference is applied. Leaving + * this `public` let each screen, widget, notification and coach path call it directly + * and drift; that is the bug the reviewer flagged. New callers use + * [com.pulseloop.data.DisplaySteps], not this. Test source sets (same module) can still + * exercise it directly. */ - fun displaySteps(preferPhone: Boolean): Int = + internal fun displaySteps(preferPhone: Boolean): Int = if (preferPhone) phoneSteps ?: steps else steps /** * The whole entity a screen should show, with `steps` projected for display under the user's * step-source preference. Only `steps` is rewritten; `distanceMeters`, `calories`, and the * rest of the row are left as the ring wrote them (Path B — see PR #98's design notes). - * Callers that hold a whole row in their state (e.g. - * [com.pulseloop.ui.viewmodels.ActivityViewModel]) use this; callers that only need the - * scalar use [displaySteps]. + * + * `internal` for the same reason as [displaySteps] — use + * [com.pulseloop.data.DisplaySteps.project]. */ - fun forDisplay(preferPhone: Boolean): ActivityDailyEntity = + internal fun forDisplay(preferPhone: Boolean): ActivityDailyEntity = if (preferPhone && phoneSteps != null) copy(steps = phoneSteps) else this } diff --git a/app/src/main/java/com/pulseloop/notifications/NotificationContextBuilder.kt b/app/src/main/java/com/pulseloop/notifications/NotificationContextBuilder.kt index 9d2c9a80..367e7d14 100644 --- a/app/src/main/java/com/pulseloop/notifications/NotificationContextBuilder.kt +++ b/app/src/main/java/com/pulseloop/notifications/NotificationContextBuilder.kt @@ -38,7 +38,7 @@ object NotificationContextBuilder { // display read site. val todayStart = localStartOfDay(now) val todayActivity = db.activityDailyDao().byDay(todayStart) - val todaySteps = todayActivity?.displaySteps(preferPhone) + val todaySteps = com.pulseloop.data.DisplaySteps.count(todayActivity, preferPhone) // Latest sleep val latestSleep = db.sleepSessionDao().recent(1).firstOrNull() diff --git a/app/src/main/java/com/pulseloop/ui/viewmodels/ViewModels.kt b/app/src/main/java/com/pulseloop/ui/viewmodels/ViewModels.kt index 79ac2818..48060e47 100644 --- a/app/src/main/java/com/pulseloop/ui/viewmodels/ViewModels.kt +++ b/app/src/main/java/com/pulseloop/ui/viewmodels/ViewModels.kt @@ -103,7 +103,7 @@ class TodayViewModel( ?: DailyCalorieEstimator.deviceReportedCalories(row) } _state.update { it.copy( - steps = activity?.displaySteps(stepSourcePrefs.preferPhone()), + steps = com.pulseloop.data.DisplaySteps.count(activity, stepSourcePrefs.preferPhone()), calories = calories, distanceMeters = activity?.distanceMeters, activeMinutes = activity?.activeMinutes, @@ -126,7 +126,7 @@ class TodayViewModel( // 7-day step series for the hero delta (oldest→newest; DAO returns newest-first). viewModelScope.launch { db.activityDailyDao().recentFlow(7).collect { days -> - _state.update { it.copy(steps7d = days.reversed().map { d -> d.displaySteps(stepSourcePrefs.preferPhone()) }) } + _state.update { it.copy(steps7d = com.pulseloop.data.DisplaySteps.counts(days.reversed(), stepSourcePrefs.preferPhone())) } } } // Today's coach summary card (kind="today", scopeKey=local date). @@ -278,8 +278,10 @@ class SleepViewModel( // aggregateCoach/averageScore take a synchronous lookup; pre-warm the cache first. val goal = try { db.userGoalDao().get()?.sleepMinutes } catch (_: Exception) { null } val steps = try { - db.activityDailyDao().byDay(TimeUtil.startOfTodayLocal()) - ?.displaySteps(stepSourcePrefs.preferPhone()) + com.pulseloop.data.DisplaySteps.count( + db.activityDailyDao().byDay(TimeUtil.startOfTodayLocal()), + stepSourcePrefs.preferPhone(), + ) } catch (_: Exception) { null } // ── Today tile (never moves with Day-view navigation) ── @@ -513,12 +515,12 @@ class ActivityViewModel( viewModelScope.launch { db.activityDailyDao().recentFlow(7).collect { days -> val preferPhone = stepSourcePrefs.preferPhone() - _state.update { it.copy(recentDays = days.map { d -> d.forDisplay(preferPhone) }) } + _state.update { it.copy(recentDays = com.pulseloop.data.DisplaySteps.project(days, preferPhone)) } } } viewModelScope.launch { currentDayValues(todayStart, db.activityDailyDao()::byDayFlow).collect { day -> - _state.update { it.copy(today = day?.forDisplay(stepSourcePrefs.preferPhone())) } + _state.update { it.copy(today = com.pulseloop.data.DisplaySteps.project(day, stepSourcePrefs.preferPhone())) } } } viewModelScope.launch { @@ -543,7 +545,7 @@ class ActivityViewModel( val activeCalories = day?.let { DailyCalorieEstimator.effectiveActiveCalories(it) } _state.update { it.copy( - daySummary = day?.forDisplay(stepSourcePrefs.preferPhone()), + daySummary = com.pulseloop.data.DisplaySteps.project(day, stepSourcePrefs.preferPhone()), effectiveCalories = calories, effectiveActiveCalories = activeCalories, ) diff --git a/app/src/main/java/com/pulseloop/widgets/WidgetSnapshotPublisher.kt b/app/src/main/java/com/pulseloop/widgets/WidgetSnapshotPublisher.kt index a1f9c747..67422ceb 100644 --- a/app/src/main/java/com/pulseloop/widgets/WidgetSnapshotPublisher.kt +++ b/app/src/main/java/com/pulseloop/widgets/WidgetSnapshotPublisher.kt @@ -202,7 +202,7 @@ object WidgetSnapshotPublisher { // Read-time column pick (Path B, PR #98): the ring's `steps` unless the user has chosen // the phone AND the phone has a value for the day. Matches the app's Today tile. - val steps = today?.displaySteps(preferPhone) + val steps = com.pulseloop.data.DisplaySteps.count(today, preferPhone) val calories = today?.calories val distanceDisplay = today?.distanceMeters?.let { UnitConverter.distance(it, units) } From 86b88756e0f59ebc647c1d09a4fa0bc3cbe4fede Mon Sep 17 00:00:00 2001 From: leona Date: Mon, 26 Oct 2026 08:52:13 +0100 Subject: [PATCH 30/33] Step source: refresh screens when the toggle changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review item 7. TodayViewModel and ActivityViewModel read stepSourcePrefs.preferPhone() inside their collect blocks, so a toggle with no accompanying DB write left the display on the old source until some unrelated row changed. Same for SleepViewModel's coach input. StepSourcePrefs.preferPhoneFlow() is a callbackFlow over OnSharedPreferenceChangeListener — emits immediately, then on every step_source write. Each ViewModel combines it with its existing DB flow, so a toggle and a data write both re-project. Only fires on KEY_STEP_SOURCE; the lastBackfillDay/hasEverSeenPhoneSteps churn doesn't re-project. distinctUntilChanged swallows a no-op write of the same value. WidgetSnapshotPublisher and NotificationContextBuilder deliberately don't get the flow: the widget republishes on the foreground edge that returning from Settings fires, and the notification context is generated per check-in slot. Neither is stale in the way Today/Activity are. --- .../com/pulseloop/settings/StepSourcePrefs.kt | 30 +++++++++++++ .../com/pulseloop/ui/viewmodels/ViewModels.kt | 42 ++++++++++++++----- 2 files changed, 61 insertions(+), 11 deletions(-) diff --git a/app/src/main/java/com/pulseloop/settings/StepSourcePrefs.kt b/app/src/main/java/com/pulseloop/settings/StepSourcePrefs.kt index cb88af8f..962c5008 100644 --- a/app/src/main/java/com/pulseloop/settings/StepSourcePrefs.kt +++ b/app/src/main/java/com/pulseloop/settings/StepSourcePrefs.kt @@ -1,6 +1,11 @@ package com.pulseloop.settings import android.content.Context +import android.content.SharedPreferences +import kotlinx.coroutines.channels.awaitClose +import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.callbackFlow +import kotlinx.coroutines.flow.distinctUntilChanged /** * The user's step-source preference ("ring" or "phone"). @@ -31,6 +36,31 @@ class StepSourcePrefs(context: Context) { */ fun preferPhone(): Boolean = stepSource == SOURCE_PHONE + /** + * [preferPhone] as a hot-observer Flow — emits the current value immediately, then again + * whenever [stepSource] is written (via `OnSharedPreferenceChangeListener`). + * + * Review item 7, PR #98. `preferPhone()` is cheap to read at each DB emission, so the + * ViewModels used to consult it inside their `.collect { }` blocks. That meant a toggle + * with no accompanying DB write left the screen stale: no flow emits, so the display + * keeps the old column until some unrelated row changes. Combining this with the DB + * flow in each ViewModel (`combine(dbFlow, preferPhoneFlow())`) makes the display + * follow the toggle, not just the data. + * + * Only fires for [KEY_STEP_SOURCE] writes — `lastBackfillDay` and + * `hasEverSeenPhoneSteps` churn is not relevant to what the user sees and must not + * re-project the display. [distinctUntilChanged] also swallows a no-op write of the + * same value (SharedPreferences fires the listener even on an unchanged `putString`). + */ + fun preferPhoneFlow(): Flow = callbackFlow { + trySend(preferPhone()) + val listener = SharedPreferences.OnSharedPreferenceChangeListener { _, key -> + if (key == KEY_STEP_SOURCE) trySend(stepSource == SOURCE_PHONE) + } + prefs.registerOnSharedPreferenceChangeListener(listener) + awaitClose { prefs.unregisterOnSharedPreferenceChangeListener(listener) } + }.distinctUntilChanged() + /** * Local-midnight epoch millis of the last day on which the phone step reader ran a * successful 30-day backfill. 0 means "never run". Read/written only from diff --git a/app/src/main/java/com/pulseloop/ui/viewmodels/ViewModels.kt b/app/src/main/java/com/pulseloop/ui/viewmodels/ViewModels.kt index 48060e47..5ad42e73 100644 --- a/app/src/main/java/com/pulseloop/ui/viewmodels/ViewModels.kt +++ b/app/src/main/java/com/pulseloop/ui/viewmodels/ViewModels.kt @@ -90,7 +90,10 @@ class TodayViewModel( init { viewModelScope.launch { - currentDayValues(todayStart, db.activityDailyDao()::byDayFlow).collect { activity -> + combine( + currentDayValues(todayStart, db.activityDailyDao()::byDayFlow), + stepSourcePrefs.preferPhoneFlow(), + ) { activity, preferPhone -> activity to preferPhone }.collect { (activity, preferPhone) -> // iOS #98: the ring's own calorie figure wins when it reported one; otherwise show // the on-device estimate (BMR accrued so far + net active). Reading `calories` // straight off the row would show 0 all day for rings that report none — which is @@ -103,7 +106,7 @@ class TodayViewModel( ?: DailyCalorieEstimator.deviceReportedCalories(row) } _state.update { it.copy( - steps = com.pulseloop.data.DisplaySteps.count(activity, stepSourcePrefs.preferPhone()), + steps = com.pulseloop.data.DisplaySteps.count(activity, preferPhone), calories = calories, distanceMeters = activity?.distanceMeters, activeMinutes = activity?.activeMinutes, @@ -125,8 +128,11 @@ class TodayViewModel( } // 7-day step series for the hero delta (oldest→newest; DAO returns newest-first). viewModelScope.launch { - db.activityDailyDao().recentFlow(7).collect { days -> - _state.update { it.copy(steps7d = com.pulseloop.data.DisplaySteps.counts(days.reversed(), stepSourcePrefs.preferPhone())) } + combine( + db.activityDailyDao().recentFlow(7), + stepSourcePrefs.preferPhoneFlow(), + ) { days, preferPhone -> days to preferPhone }.collect { (days, preferPhone) -> + _state.update { it.copy(steps7d = com.pulseloop.data.DisplaySteps.counts(days.reversed(), preferPhone)) } } } // Today's coach summary card (kind="today", scopeKey=local date). @@ -264,7 +270,13 @@ class SleepViewModel( init { // Any sleep-table change retriggers a rebuild of the selected range. viewModelScope.launch { - db.sleepSessionDao().recentFlow(7).collect { sessions -> + // Combined with the step-source preference (review item 7, PR #98) so the sleep + // coach's step figure follows the toggle without waiting for a sleep-table write. + // rebuild() reads preferPhone() fresh each call, so the emit is the entire fix. + combine( + db.sleepSessionDao().recentFlow(7), + stepSourcePrefs.preferPhoneFlow(), + ) { sessions, _ -> sessions }.collect { sessions -> _state.update { it.copy(recentSessions = sessions) } try { rebuild(_state.value.range) } catch (_: Exception) {} } @@ -513,18 +525,26 @@ class ActivityViewModel( init { viewModelScope.launch { - db.activityDailyDao().recentFlow(7).collect { days -> - val preferPhone = stepSourcePrefs.preferPhone() + combine( + db.activityDailyDao().recentFlow(7), + stepSourcePrefs.preferPhoneFlow(), + ) { days, preferPhone -> days to preferPhone }.collect { (days, preferPhone) -> _state.update { it.copy(recentDays = com.pulseloop.data.DisplaySteps.project(days, preferPhone)) } } } viewModelScope.launch { - currentDayValues(todayStart, db.activityDailyDao()::byDayFlow).collect { day -> - _state.update { it.copy(today = com.pulseloop.data.DisplaySteps.project(day, stepSourcePrefs.preferPhone())) } + combine( + currentDayValues(todayStart, db.activityDailyDao()::byDayFlow), + stepSourcePrefs.preferPhoneFlow(), + ) { day, preferPhone -> day to preferPhone }.collect { (day, preferPhone) -> + _state.update { it.copy(today = com.pulseloop.data.DisplaySteps.project(day, preferPhone)) } } } viewModelScope.launch { - shownDayStart.flatMapLatest { day -> db.activityDailyDao().byDayFlow(day) }.collect { day -> + combine( + shownDayStart.flatMapLatest { day -> db.activityDailyDao().byDayFlow(day) }, + stepSourcePrefs.preferPhoneFlow(), + ) { day, preferPhone -> day to preferPhone }.collect { (day, preferPhone) -> // Effective calories for the shown day — same read path as the Today card // (see TodayViewModel's init), so both screens agree. Reading `day.calories` // straight off the row would surface the ring's value for ring modes, but @@ -545,7 +565,7 @@ class ActivityViewModel( val activeCalories = day?.let { DailyCalorieEstimator.effectiveActiveCalories(it) } _state.update { it.copy( - daySummary = com.pulseloop.data.DisplaySteps.project(day, stepSourcePrefs.preferPhone()), + daySummary = com.pulseloop.data.DisplaySteps.project(day, preferPhone), effectiveCalories = calories, effectiveActiveCalories = activeCalories, ) From 4abc45cabd8f61017073c81ed63cb7a80bf469c4 Mon Sep 17 00:00:00 2001 From: leona Date: Mon, 26 Oct 2026 08:57:46 +0100 Subject: [PATCH 31/33] Phone steps: paginate foreignStepOrigins MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review item 8. ReadRecordsRequest returns at most one page (default pageSize 1000) and puts the cursor in pageToken. The old code read a single page and ignored the token, so an origin that first appeared after the 1000th record was silently missing from dataOriginFilter — that source's steps were under-counted for the whole range, and a source with only a handful of records could be dropped entirely. Loop until pageToken is null. pageSize bumped to 5000 (the SDK max) to keep a heavy 30-day range in one round trip. Also updates the KDoc to explain why the raw-record read is unavoidable: HC has no origins-enumeration API, so both aggregate variants need a dataOriginFilter up front. And it warns future readers off the seemingly-smarter alternatives (unfiltered aggregate, or aggregate-minus-own-aggregate) — HC de-duplicates by priority, not additively, so either shortcut produces wrong totals. That's the mechanism behind review item 1 in the previous round. --- .../java/com/pulseloop/PhoneStepManager.kt | 37 ++++++++++++++++--- 1 file changed, 31 insertions(+), 6 deletions(-) diff --git a/app/src/main/java/com/pulseloop/PhoneStepManager.kt b/app/src/main/java/com/pulseloop/PhoneStepManager.kt index 505c88ff..dce98a74 100644 --- a/app/src/main/java/com/pulseloop/PhoneStepManager.kt +++ b/app/src/main/java/com/pulseloop/PhoneStepManager.kt @@ -228,18 +228,43 @@ class PhoneStepManager(private val context: Context) { * The set of Health Connect origins writing `StepsRecord`s in [timeRangeFilter], minus * PulseLoop's own. The result is passed as `dataOriginFilter` to the aggregates above so * HC's priority-based de-duplication runs over foreign sources only — see the class KDoc. + * + * **Why this reads raw records (review item 8, PR #98).** Health Connect has no API that + * enumerates "the set of origins writing a record type" — `AggregateRequest` and + * `AggregateGroupByPeriodRequest` both require a `dataOriginFilter` *up front*, and + * there is nothing else in the SDK to discover it. Reading records and inspecting each + * `metadata.dataOrigin` is the only option; the cost is intrinsic to the API. Do not + * "optimise" this by swapping in an unfiltered aggregate or by subtracting an own-only + * aggregate — HC's de-duplication is priority-based, not additive (see the class KDoc), + * so either shortcut produces wrong totals. + * + * **Pagination.** [ReadRecordsRequest] returns one page at a time (default `pageSize` + * of 1000) and puts the cursor in `pageToken`. The previous code read a single page and + * ignored the token, so an origin that first appeared *after* the first 1000 records + * was silently excluded from `dataOriginFilter` — under-counting that source's steps + * for the whole range. Loop until `pageToken` is null. In practice a 30-day backfill + * window fits in one page for a typical step writer, so the loop collapses to one + * round trip; the `pageSize` bump keeps that true for heavier ranges too. */ private suspend fun foreignStepOrigins( client: HealthConnectClient, timeRangeFilter: TimeRangeFilter, ): Set { - val records = client.readRecords( - ReadRecordsRequest( - recordType = StepsRecord::class, - timeRangeFilter = timeRangeFilter, + val origins = mutableSetOf() + var pageToken: String? = null + do { + val response = client.readRecords( + ReadRecordsRequest( + recordType = StepsRecord::class, + timeRangeFilter = timeRangeFilter, + pageSize = 5_000, + pageToken = pageToken, + ) ) - ).records - return records.map { it.metadata.dataOrigin }.toSet() - ownOrigin + response.records.forEach { origins.add(it.metadata.dataOrigin) } + pageToken = response.pageToken + } while (pageToken != null) + return origins - ownOrigin } companion object { From 019df687fd1a08322ac11562b873b95622251090 Mon Sep 17 00:00:00 2001 From: leona Date: Mon, 26 Oct 2026 09:04:09 +0100 Subject: [PATCH 32/33] Activity: show Records on phone-mode days the phone never backfilled MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review item 9. The guard hid the RECORDS card whenever the step-source preference was Phone. That is broader than the reason it exists: the card is hidden because on a phone-owned day the ring's bucket list no longer sums to the phone total in the header above it, which reads as a bug. On a phone-mode day whose phoneSteps is null — pre-backfill, or a phone with no step writer, the case review item 3 covers — the header falls back to the ring's total and the ring's own buckets are exactly the records behind it. Hiding the card there made a bad ring bucket undeletable. phoneOwnsShownDay asks the two questions that actually determine whether the card would disagree with its header: the preference is Phone, and the phone wrote a value for the day. state.daySummary is already projected, so its phoneSteps field is the raw column, untouched by projection — a phone-mode day that fell back to the ring has phoneSteps = null and phoneOwnsShownDay is false. Both the KDoc and the comment are rewritten: the old ones described the guard as if it were about "phone mode" in the abstract, which is exactly the assumption the fix rejects. --- .../pulseloop/ui/screens/ActivityScreen.kt | 21 +++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/app/src/main/java/com/pulseloop/ui/screens/ActivityScreen.kt b/app/src/main/java/com/pulseloop/ui/screens/ActivityScreen.kt index 8a50a5a2..0f3c13fd 100644 --- a/app/src/main/java/com/pulseloop/ui/screens/ActivityScreen.kt +++ b/app/src/main/java/com/pulseloop/ui/screens/ActivityScreen.kt @@ -236,12 +236,21 @@ fun ActivityScreen( // than beside it: it answers "why is that number wrong", which is a question you only ask // after reading the number. // - // Hidden when the step source is "Phone": the Records list is the ring's intraday - // bucket log, and under Path B the header above it is the phone's total (chosen at - // read time from `phoneSteps`), so the records no longer sum to the number they sit - // under — reading as a bug rather than a detail view. Deleting a ring bucket would - // also not move the phone-sourced total. Toggling back to "Ring" restores the card. - if (stepSourcePrefs.stepSource != com.pulseloop.settings.StepSourcePrefs.SOURCE_PHONE) { + // Hidden only when the phone is *actually supplying the shown day's steps* (review + // item 9, PR #98). The old guard keyed on the user's preference alone, which hid the + // card even on days whose `phoneSteps` is null — those days display the ring's total, + // so the ring's own buckets are the right records for the number above them, and a + // bad bucket in that total needs to be deletable. `phoneOwnsShownDay` asks the two + // questions that actually determine it: the preference is Phone, AND the phone wrote + // a value for this day. `state.daySummary` is already projected by the ViewModel, so + // its `phoneSteps` field is the raw column — untouched by projection. + // + // On a phone-owned day the records legitimately do not sum to the header (ring + // buckets vs. phone total), which reads as a bug rather than a detail view — hide it + // for that case. Toggling back to Ring, or landing on a day the phone never + // backfilled, restores the card. + val phoneOwnsShownDay = stepSourcePrefs.preferPhone() && state.daySummary?.phoneSteps != null + if (!phoneOwnsShownDay) { item { ActivityRecordsCard( viewModel = viewModel, From 6efa020da7068bb532ae6f26bb3452a5cab4c846 Mon Sep 17 00:00:00 2001 From: leona Date: Mon, 26 Oct 2026 09:13:59 +0100 Subject: [PATCH 33/33] Review cleanup: stale KDoc, mis-indented comment, wrong test filename MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three small fixes from the reviewer's "Minor" list, no behavior change. upsertActivityDailyFromPhone's KDoc claimed "Later ring writes preserve that source", but upsertActivityDaily flips source from "phone" to "ring" — deliberately, because source means "who last wrote the data columns" and once the ring has supplied steps/calories/distance the row belongs to the ring. The KDoc now describes what the code does and why the separate phoneSteps column, not the source label, is what keeps the phone's value visible. computeSleepScore's comment continuation was indented to 4 spaces on its first line and 8 on its second. Both to 8, matching the surrounding expression style. PhoneStepSourceTest's KDoc pointed at `EventPersistenceSubscriberPhoneWriteTest.kt`, which does not exist. The actual file is `EventPersistencePhoneWriteTest.kt` (no "Subscriber"). Fixed. The reviewer's fourth minor item — "Lines unrelated to the feature were re-indented in several files" — is not addressed here. The reindents are in earlier commits (pre-PR-history churn that predates this session), and reverting them blind risks introducing more whitespace noise than it removes. Calling it out in the PR summary instead, with an offer to revert specific commits if the reviewer points at them. --- .../pulseloop/service/EventPersistenceSubscriber.kt | 13 +++++++++---- .../com/pulseloop/service/PhoneStepSourceTest.kt | 2 +- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/app/src/main/java/com/pulseloop/service/EventPersistenceSubscriber.kt b/app/src/main/java/com/pulseloop/service/EventPersistenceSubscriber.kt index ab95271f..48bf110a 100644 --- a/app/src/main/java/com/pulseloop/service/EventPersistenceSubscriber.kt +++ b/app/src/main/java/com/pulseloop/service/EventPersistenceSubscriber.kt @@ -575,9 +575,14 @@ class EventPersistenceSubscriber( * guards skip it regardless of source. A phone-first row also stamps `updatedAt = now` * (see [phoneStepRow]) so the exporter's watermark query picks it up. * - * Later ring writes preserve that source — see [upsertActivityDaily]. The old "overwrite - * source and reset it back to ring on next ring write" dance is gone; nothing needs it any - * more. + * A later ring write flips `source` from `"phone"` to `"ring"` — see [upsertActivityDaily]. + * That is not a mismatch: `source` means "who last wrote the data columns", and once the + * ring has supplied `steps`/`calories`/`distance`, the row belongs to the ring. + * [DailyCalorieEstimator.deviceReportedCalories] reads `source` for exactly that question. + * The phone's step value is kept visible by the separate `phoneSteps` column, not by the + * source label — the old "overwrite source and reset it back to ring on next ring write" + * dance existed because the phone used to occupy `steps`; under the separate-column design + * nothing needs it any more. * * A no-op when the day already holds this exact phone value: on every foreground the phone * reader re-publishes today's total, so a value that hasn't moved must not produce a write. @@ -861,7 +866,7 @@ class EventPersistenceSubscriber( * Matches the official app's scoring. */ private fun computeSleepScore(deepMin: Int, totalMin: Int): Int? = - // Delegates since issue #78: SleepRecordDeletion restates deleted-from sessions with the + // Delegates since issue #78: SleepRecordDeletion restates deleted-from sessions with the // same banding, so one function owns it. sleepStageScore(deepMin, totalMin) diff --git a/app/src/test/java/com/pulseloop/service/PhoneStepSourceTest.kt b/app/src/test/java/com/pulseloop/service/PhoneStepSourceTest.kt index 0a345060..7109dc38 100644 --- a/app/src/test/java/com/pulseloop/service/PhoneStepSourceTest.kt +++ b/app/src/test/java/com/pulseloop/service/PhoneStepSourceTest.kt @@ -20,7 +20,7 @@ import java.time.Instant * The read-time source selection itself (`ActivityDailyEntity.displaySteps` / `forDisplay`) is * covered in `app/src/test/java/com/pulseloop/data/ActivityDailyDisplayTest.kt`. The phone * write's column-only behaviour is covered (Room-backed) in - * `app/src/test/java/com/pulseloop/service/EventPersistenceSubscriberPhoneWriteTest.kt`. + * `app/src/test/java/com/pulseloop/service/EventPersistencePhoneWriteTest.kt`. * The EXCLUDED_SOURCES assertion that used to live here was superseded by * `HealthConnectTypeMappingsTest.excludedSourcesCoversDemoAndMock` in commit 41beee0 — `"phone"` * was dropped from the set, so the old assertion was simply wrong.