From d504f418708af8e040a2f87e3681885a3d2664dc Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 12 Sep 2026 13:52:19 +0000 Subject: [PATCH] fix: replace the whole form when a second workout suggestion is picked MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three defects found auditing the Health Connect workout path. applyPrefill only assigned a field when the incoming route carried a value, so picking a second suggestion merged into the first instead of replacing it. Tapping a run (5 km, 380 kcal) and then a gym session left the run's distance and calories in the form — the user publishes numbers from a workout that never happened. Every metric is now assigned on both branches. Notes are deliberately left alone: they are typed by the user, never carried by a route. The `source` tag published the writing app's display label ("Samsung Health"), or its raw package name when that app is not installed. SourceTag defines a vocabulary — gps / manual / health_connect — and the feed badge uppercases whatever is in the tag, so an imported workout rendered as "SAMSUNG HEALTH" or "COM.HUAWEI.HEALTH" beside other clients' "GPS". It now publishes SourceTag.HEALTH_CONNECT, which existed for this and had no callers. DetectedWorkout.source keeps the friendly name for the UI. readNewWorkouts ran entirely on Dispatchers.Main: the callers launch into a composable's rememberCoroutineScope, and nothing in the feature switched dispatcher. Health Connect's own calls suspend, but resolveSourceName's PackageManager lookup is a blocking binder call made once per session, so a week of sessions blocked the UI thread once each. The read now runs on Dispatchers.IO and the label lookups are memoized per writer package. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019egdJyBHnrATZHjs86up8f --- .../workouts/health/HealthConnectManager.kt | 67 ++++++---- .../loggedIn/workouts/NewWorkoutViewModel.kt | 14 ++- .../suggestion/WorkoutSuggestionShared.kt | 7 +- .../workouts/NewWorkoutPrefillTest.kt | 114 ++++++++++++++++++ 4 files changed, 175 insertions(+), 27 deletions(-) create mode 100644 amethyst/src/test/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/workouts/NewWorkoutPrefillTest.kt diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/workouts/health/HealthConnectManager.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/workouts/health/HealthConnectManager.kt index 051c914ec5..8ffd1397d7 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/workouts/health/HealthConnectManager.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/workouts/health/HealthConnectManager.kt @@ -36,6 +36,8 @@ import androidx.health.connect.client.request.ReadRecordsRequest import androidx.health.connect.client.time.TimeRangeFilter import com.vitorpamplona.quartz.utils.Log import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.withContext import java.time.Duration import java.time.Instant import kotlin.math.roundToInt @@ -54,6 +56,9 @@ class HealthConnectManager( ) { private val client: HealthConnectClient by lazy { HealthConnectClient.getOrCreate(context) } + /** Writer package -> display label. See [resolveSourceName]. */ + private val sourceNames = mutableMapOf() + companion object { private const val TAG = "HealthConnectManager" @@ -132,25 +137,31 @@ class HealthConnectManager( return emptyList() } - return try { - val response = - client.readRecords( - ReadRecordsRequest( - recordType = ExerciseSessionRecord::class, - timeRangeFilter = TimeRangeFilter.between(since, now), - ), - ) - Log.i(TAG) { "readNewWorkouts: ${response.records.size} exercise session(s) in window $since .. $now" } - val mapped = response.records.mapNotNull { mapSession(it) } - // Fold split-up sessions of the same activity (a long run broken around - // breaks) into one suggestion so the composer offers the whole effort. - val merged = WorkoutMerger.mergeCloseWorkouts(mapped) - Log.i(TAG) { "readNewWorkouts: mapped ${mapped.size} -> ${merged.size} workout(s) after type/duration filtering and merging" } - merged - } catch (e: Exception) { - if (e is CancellationException) throw e - Log.w(TAG, "Failed to read workouts from Health Connect", e) - emptyList() + // The callers are composables launching into rememberCoroutineScope(), i.e. + // Dispatchers.Main. Health Connect's own calls suspend, but the PackageManager + // lookup in resolveSourceName is a blocking binder call, so the whole read + // moves off the UI thread rather than relying on each step to behave. + return withContext(Dispatchers.IO) { + try { + val response = + client.readRecords( + ReadRecordsRequest( + recordType = ExerciseSessionRecord::class, + timeRangeFilter = TimeRangeFilter.between(since, now), + ), + ) + Log.i(TAG) { "readNewWorkouts: ${response.records.size} exercise session(s) in window $since .. $now" } + val mapped = response.records.mapNotNull { mapSession(it) } + // Fold split-up sessions of the same activity (a long run broken around + // breaks) into one suggestion so the composer offers the whole effort. + val merged = WorkoutMerger.mergeCloseWorkouts(mapped) + Log.i(TAG) { "readNewWorkouts: mapped ${mapped.size} -> ${merged.size} workout(s) after type/duration filtering and merging" } + merged + } catch (e: Exception) { + if (e is CancellationException) throw e + Log.w(TAG, "Failed to read workouts from Health Connect", e) + emptyList() + } } } @@ -208,11 +219,19 @@ class HealthConnectManager( */ private fun resolveSourceName(packageName: String): String { if (packageName.isBlank()) return DEFAULT_SOURCE - runCatching { - val pm = context.packageManager - return pm.getApplicationLabel(pm.getApplicationInfo(packageName, 0)).toString() - } - return KNOWN_SOURCES[packageName] ?: packageName + // Memoized: getApplicationInfo is a blocking binder call, and a week of sessions + // almost always comes from the same one or two writer apps, so an uncached lookup + // pays for the same round trip once per session. + sourceNames[packageName]?.let { return it } + + val resolved = + runCatching { + val pm = context.packageManager + pm.getApplicationLabel(pm.getApplicationInfo(packageName, 0)).toString() + }.getOrNull() ?: KNOWN_SOURCES[packageName] ?: packageName + + sourceNames[packageName] = resolved + return resolved } /** Aggregates the optional metrics over the session window. Null if aggregation fails. */ diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/workouts/NewWorkoutViewModel.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/workouts/NewWorkoutViewModel.kt index 3a0843eaf1..535ac8d5de 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/workouts/NewWorkoutViewModel.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/workouts/NewWorkoutViewModel.kt @@ -93,20 +93,30 @@ class NewWorkoutViewModel : ViewModel() { */ fun applyPrefill(route: Route.NewWorkout) { route.exercise?.let { ExerciseType.parse(it) }?.let { exercise = it } - route.title?.let { title = it } + title = route.title ?: "" + // Every metric is assigned on both branches. Picking a second suggestion has to + // REPLACE the form, not merge into it: a gym session carries no distance, so + // leaving the previous run's 5 km in the field would publish a workout that + // never happened. if (route.durationSeconds > 0) { hours = (route.durationSeconds / 3600).toString() minutes = ((route.durationSeconds % 3600) / 60).toString() seconds = (route.durationSeconds % 60).toString() + } else { + hours = "" + minutes = "" + seconds = "" } if (route.distanceMeters > 0) { val miles = phonePrefersMiles() distanceUnit = if (miles) DistanceTag.MILES else DistanceTag.KILOMETERS val value = if (miles) route.distanceMeters / DistanceTag.METERS_PER_MILE else route.distanceMeters / 1000.0 distance = ((value * 100).roundToInt() / 100.0).toString() + } else { + distance = "" } - if (route.calories > 0) calories = route.calories.toString() + calories = if (route.calories > 0) route.calories.toString() else "" route.source?.let { source = it } avgHeartRate = route.avgHeartRate diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/workouts/suggestion/WorkoutSuggestionShared.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/workouts/suggestion/WorkoutSuggestionShared.kt index 9cb951f49e..5030b4b872 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/workouts/suggestion/WorkoutSuggestionShared.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/workouts/suggestion/WorkoutSuggestionShared.kt @@ -23,6 +23,7 @@ package com.vitorpamplona.amethyst.ui.screen.loggedIn.workouts.suggestion import android.text.format.DateUtils import com.vitorpamplona.amethyst.service.workouts.health.DetectedWorkout import com.vitorpamplona.amethyst.ui.navigation.routes.Route +import com.vitorpamplona.quartz.experimental.fitness.workout.tags.SourceTag /** Builds the pre-filled composer route for a detected workout. Shared by the * feed suggestion banner and the New Workout carousel so they never drift. */ @@ -38,7 +39,11 @@ internal fun DetectedWorkout.toNewWorkoutRoute(title: String) = steps = steps ?: 0, elevationGainMeters = elevationGainMeters ?: 0.0, startTime = startTimeEpochSeconds, - source = source, + // The `source` tag carries the NIP-101e vocabulary token, not the writing app's + // label: the badge in the feed renders it uppercased next to "GPS" and "MANUAL", + // and other clients match on the token. DetectedWorkout.source stays the friendly + // name for the UI to show. + source = SourceTag.HEALTH_CONNECT, ) internal fun formatWorkoutDuration(totalSeconds: Long): String { diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/workouts/NewWorkoutPrefillTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/workouts/NewWorkoutPrefillTest.kt new file mode 100644 index 0000000000..8d9fcf289e --- /dev/null +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/workouts/NewWorkoutPrefillTest.kt @@ -0,0 +1,114 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.amethyst.ui.screen.loggedIn.workouts + +import com.vitorpamplona.amethyst.service.workouts.health.DetectedWorkout +import com.vitorpamplona.amethyst.ui.navigation.routes.Route +import com.vitorpamplona.amethyst.ui.screen.loggedIn.workouts.suggestion.toNewWorkoutRoute +import com.vitorpamplona.quartz.experimental.fitness.workout.tags.ExerciseType +import com.vitorpamplona.quartz.experimental.fitness.workout.tags.SourceTag +import org.junit.Assert.assertEquals +import org.junit.Test + +class NewWorkoutPrefillTest { + private fun route( + exercise: ExerciseType, + title: String, + durationSeconds: Long, + distanceMeters: Double = 0.0, + calories: Int = 0, + source: String = "Samsung Health", + ) = Route.NewWorkout( + exercise = exercise.code, + title = title, + durationSeconds = durationSeconds, + distanceMeters = distanceMeters, + calories = calories, + avgHeartRate = 0, + maxHeartRate = 0, + steps = 0, + elevationGainMeters = 0.0, + startTime = 1_700_000_000L, + source = source, + ) + + /** + * Picking a second suggestion must replace the form, not merge into it. A gym + * session has no distance and no calories; if the run's numbers survive, the + * user publishes a workout that never happened. + */ + @Test + fun `picking a second workout clears metrics the new one does not have`() { + val vm = NewWorkoutViewModel() + + vm.applyPrefill(route(ExerciseType.RUNNING, "Morning Run", 1800, distanceMeters = 5000.0, calories = 380)) + assertEquals("380", vm.calories) + + vm.applyPrefill(route(ExerciseType.STRENGTH, "Gym", 2400)) + + assertEquals(ExerciseType.STRENGTH, vm.exercise) + assertEquals("Gym", vm.title) + assertEquals("", vm.distance) + assertEquals("", vm.calories) + } + + /** + * A Health Connect import publishes the NIP-101e vocabulary token, not the writing + * app's label: the feed badge uppercases whatever is in the tag, so a raw label + * renders as "SAMSUNG HEALTH" (or "COM.HUAWEI.HEALTH" when the app is not installed) + * next to other clients' "GPS" and "MANUAL". + */ + @Test + fun `a detected workout carries the health_connect source token`() { + val detected = + DetectedWorkout( + id = "abc", + exercise = ExerciseType.RUNNING, + title = "Morning Run", + startTimeEpochSeconds = 1_700_000_000L, + durationSeconds = 1800, + distanceMeters = 5000.0, + calories = 380, + avgHeartRate = 150, + maxHeartRate = 172, + steps = 6000, + elevationGainMeters = 40.0, + source = "Samsung Health", + ) + + assertEquals(SourceTag.HEALTH_CONNECT, detected.toNewWorkoutRoute("Morning Run").source) + } + + /** Duration must switch too, not keep the previous workout's clock. */ + @Test + fun `picking a second workout replaces the duration`() { + val vm = NewWorkoutViewModel() + + vm.applyPrefill(route(ExerciseType.RUNNING, "Morning Run", 3661)) + assertEquals("1", vm.hours) + + vm.applyPrefill(route(ExerciseType.YOGA, "Yoga", 600)) + + assertEquals("0", vm.hours) + assertEquals("10", vm.minutes) + assertEquals("0", vm.seconds) + } +}