From ccac827d5fae384e24c726757f66e5fbeff492a3 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 17 Sep 2026 15:53:00 +0000 Subject: [PATCH] =?UTF-8?q?fix:=20code-review=20pass=20=E2=80=94=20POWR=20?= =?UTF-8?q?durations,=20pinned=20bottom=20bar,=20carousel=20duplicates?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CONTRIBUTING-WITH-AI requires a review pass by a different agent before opening the PR. It found five issues; four are fixed here. toDetectedWorkout read durationSeconds(), so a POWR-dialect kind 1301 — which carries start/end and no duration tag — mapped to null and disappeared from the dashboard entirely. It now reads effectiveDurationSeconds(), the helper WorkoutDisplay already uses, and the function was already falling back to workoutStart() for the timestamp, so it was half-converted. Covered by a test built from start/end tags alone. MyFitnessScreen used a bare Scaffold while MY_FITNESS is pinnable to the bottom bar, so pinning it and tapping it made the bar vanish. Now DisappearingScaffold + AppBottomBar, matching Drafts and every other pinnable destination. The New Workout carousel still offered Health Connect workouts the user had already shared — the alreadyPublished flag was honoured only in My Fitness, so the duplicate-publish path fixed two commits ago stayed open one screen over. Both screens now build their "what have I already posted" view from one shared helper, publishedWorkoutsOf, because two answers to that question is what let them disagree in the first place. Also corrects the declaration's "Related code" paths, which still pointed at service/workouts/health for two files that now live in commons/fitness. The fifth finding is NOT fixed and is called out in the PR description: the dashboard never REQs kind 1301, so on a cold start it summarises only what some other screen happened to load. Fixing it means choosing outbox relays, which I cannot verify without a device or relay, and guessing at relay selection is worse than not fetching. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019egdJyBHnrATZHjs86up8f --- .../workouts/health/PublishedWorkouts.kt | 53 +++++++++++++++++++ .../workouts/fitness/MyFitnessScreen.kt | 15 +++++- .../workouts/fitness/MyFitnessViewModel.kt | 28 +--------- .../suggestion/DetectedWorkoutCarousel.kt | 13 ++++- .../amethyst/commons/fitness/TrainingLog.kt | 5 +- .../fitness/PublishedWorkoutMappingTest.kt | 20 +++++++ docs/health-connect-play-declaration.md | 6 +-- 7 files changed, 107 insertions(+), 33 deletions(-) create mode 100644 amethyst/src/main/java/com/vitorpamplona/amethyst/service/workouts/health/PublishedWorkouts.kt diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/workouts/health/PublishedWorkouts.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/workouts/health/PublishedWorkouts.kt new file mode 100644 index 0000000000..ad2d5fdfe5 --- /dev/null +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/workouts/health/PublishedWorkouts.kt @@ -0,0 +1,53 @@ +/* + * 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.service.workouts.health + +import com.vitorpamplona.amethyst.commons.fitness.DetectedWorkout +import com.vitorpamplona.amethyst.commons.fitness.toDetectedWorkout +import com.vitorpamplona.amethyst.model.LocalCache +import com.vitorpamplona.quartz.experimental.fitness.workout.WorkoutRecordEvent +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.withContext + +/** + * The workouts [pubkeyHex] has published, from [sinceEpochSeconds] onwards, as the cache currently + * holds them. + * + * Shared by the My Fitness dashboard, which counts them, and the New Workout carousel, which uses + * them to avoid offering a workout the user already shared. Both need the same view of "what have + * I already posted", and two answers to that question would mean the carousel offering something + * the dashboard knows is a duplicate. + * + * Scans off the main thread: LocalCache holds every event the session has seen and this walks all + * of them. It reports only what is already cached — it issues no REQ of its own. + */ +suspend fun publishedWorkoutsOf( + pubkeyHex: String, + sinceEpochSeconds: Long, +): List = + withContext(Dispatchers.Default) { + LocalCache.notes + .filterIntoSet { _, note -> + val event = note.event + event is WorkoutRecordEvent && event.pubKey == pubkeyHex + }.mapNotNull { (it.event as WorkoutRecordEvent).toDetectedWorkout() } + .filter { it.startTimeEpochSeconds >= sinceEpochSeconds } + } diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/workouts/fitness/MyFitnessScreen.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/workouts/fitness/MyFitnessScreen.kt index ef3405c8e9..50c251dce1 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/workouts/fitness/MyFitnessScreen.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/workouts/fitness/MyFitnessScreen.kt @@ -38,7 +38,6 @@ import androidx.compose.material3.Button import androidx.compose.material3.CircularProgressIndicator import androidx.compose.material3.MaterialTheme import androidx.compose.material3.OutlinedCard -import androidx.compose.material3.Scaffold import androidx.compose.material3.Surface import androidx.compose.material3.Text import androidx.compose.material3.TextButton @@ -94,7 +93,10 @@ import com.vitorpamplona.amethyst.commons.resources.my_fitness_window_note import com.vitorpamplona.amethyst.commons.resources.my_fitness_workouts import com.vitorpamplona.amethyst.commons.resources.workout_suggestion_connect_details import com.vitorpamplona.amethyst.service.workouts.health.HealthConnectManager +import com.vitorpamplona.amethyst.ui.layouts.DisappearingScaffold +import com.vitorpamplona.amethyst.ui.navigation.bottombars.AppBottomBar import com.vitorpamplona.amethyst.ui.navigation.navs.INav +import com.vitorpamplona.amethyst.ui.navigation.routes.Route import com.vitorpamplona.amethyst.ui.navigation.topbars.TopBarWithBackButton import com.vitorpamplona.amethyst.ui.screen.loggedIn.AccountViewModel import com.vitorpamplona.amethyst.ui.screen.loggedIn.workouts.health.HealthConnectRationaleActivity @@ -141,8 +143,17 @@ fun MyFitnessScreen( val openRationale = { context.startActivity(Intent(context, HealthConnectRationaleActivity::class.java)) } val requestPermissions = { permissionLauncher.launch(HealthConnectManager.PERMISSIONS) } - Scaffold( + // DisappearingScaffold + AppBottomBar, like every other pinnable destination: My Fitness can + // be pinned to the bottom bar, and a bare Scaffold would make that bar vanish on arrival. + DisappearingScaffold( + isInvertedLayout = false, topBar = { TopBarWithBackButton(stringRes(Res.string.my_fitness_title), nav) }, + bottomBar = { + AppBottomBar(Route.MyFitness, nav, accountViewModel) { route -> + if (route != Route.MyFitness) nav.navBottomBar(route) + } + }, + accountViewModel = accountViewModel, ) { padding -> Surface(modifier = Modifier.padding(padding)) { when (val current = state) { diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/workouts/fitness/MyFitnessViewModel.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/workouts/fitness/MyFitnessViewModel.kt index a893649434..3f829aed08 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/workouts/fitness/MyFitnessViewModel.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/workouts/fitness/MyFitnessViewModel.kt @@ -25,19 +25,14 @@ import androidx.compose.runtime.Immutable import androidx.compose.runtime.Stable import androidx.lifecycle.ViewModel import androidx.lifecycle.viewModelScope -import com.vitorpamplona.amethyst.commons.fitness.DetectedWorkout import com.vitorpamplona.amethyst.commons.fitness.TrainingLog import com.vitorpamplona.amethyst.commons.fitness.WorkoutStats -import com.vitorpamplona.amethyst.commons.fitness.toDetectedWorkout -import com.vitorpamplona.amethyst.model.LocalCache import com.vitorpamplona.amethyst.service.workouts.health.HealthConnectManager -import com.vitorpamplona.quartz.experimental.fitness.workout.WorkoutRecordEvent -import kotlinx.coroutines.Dispatchers +import com.vitorpamplona.amethyst.service.workouts.health.publishedWorkoutsOf import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.asStateFlow import kotlinx.coroutines.launch -import kotlinx.coroutines.withContext import java.time.Duration import java.time.Instant @@ -116,7 +111,7 @@ class MyFitnessViewModel : ViewModel() { emptyList() } - val fromRelays = publishedWorkouts(since.epochSecond) + val fromRelays = publishedWorkoutsOf(pubkeyHex ?: return@launch, since.epochSecond) _state.value = State.Ready( @@ -132,23 +127,4 @@ class MyFitnessViewModel : ViewModel() { val hc = manager ?: HealthConnectManager(context.applicationContext).also { manager = it } return if (hc.hasAllPermissions()) HealthConnectStatus.CONNECTED else HealthConnectStatus.AVAILABLE } - - /** - * The user's own kind 1301 events from the local cache, newer than [sinceEpochSeconds]. - * - * Scans off the main thread: LocalCache holds every event the session has seen, and this - * walks all of them. - */ - private suspend fun publishedWorkouts(sinceEpochSeconds: Long): List { - val mine = pubkeyHex ?: return emptyList() - - return withContext(Dispatchers.Default) { - LocalCache.notes - .filterIntoSet { _, note -> - val event = note.event - event is WorkoutRecordEvent && event.pubKey == mine - }.mapNotNull { (it.event as WorkoutRecordEvent).toDetectedWorkout() } - .filter { it.startTimeEpochSeconds >= sinceEpochSeconds } - } - } } diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/workouts/suggestion/DetectedWorkoutCarousel.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/workouts/suggestion/DetectedWorkoutCarousel.kt index 3258c2223f..1f29d64ecd 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/workouts/suggestion/DetectedWorkoutCarousel.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/workouts/suggestion/DetectedWorkoutCarousel.kt @@ -58,6 +58,8 @@ import androidx.lifecycle.compose.LifecycleResumeEffect import androidx.lifecycle.compose.collectAsStateWithLifecycle import com.vitorpamplona.amethyst.R import com.vitorpamplona.amethyst.commons.fitness.DetectedWorkout +import com.vitorpamplona.amethyst.commons.fitness.TrainingLog +import com.vitorpamplona.amethyst.commons.fitness.WorkoutOrigin import com.vitorpamplona.amethyst.commons.icons.symbols.Icon import com.vitorpamplona.amethyst.commons.icons.symbols.MaterialSymbols import com.vitorpamplona.amethyst.commons.resources.Res @@ -69,6 +71,7 @@ import com.vitorpamplona.amethyst.commons.resources.workout_suggestion_connect_t import com.vitorpamplona.amethyst.commons.resources.workout_suggestion_distance_km import com.vitorpamplona.amethyst.model.BooleanType import com.vitorpamplona.amethyst.service.workouts.health.HealthConnectManager +import com.vitorpamplona.amethyst.service.workouts.health.publishedWorkoutsOf import com.vitorpamplona.amethyst.ui.navigation.routes.Route import com.vitorpamplona.amethyst.ui.screen.loggedIn.AccountViewModel import com.vitorpamplona.amethyst.ui.screen.loggedIn.workouts.health.HealthConnectRationaleActivity @@ -108,13 +111,21 @@ fun DetectedWorkoutCarousel( var granted by remember { mutableStateOf(null) } var workouts by remember { mutableStateOf>(emptyList()) } + val myPubkey = accountViewModel.userProfile().pubkeyHex + val reload: suspend () -> Unit = { val ok = manager.hasAllPermissions() granted = ok workouts = if (ok) { val since = Instant.now().minus(Duration.ofDays(HealthConnectManager.LOOKBACK_DAYS)) - manager.readWorkouts(since).sortedByDescending { it.startTimeEpochSeconds } + // Offering a workout the user already shared would publish a second kind 1301 for + // the same effort. merge() flags the Health Connect copies that match something + // already posted, so drop those; what is left is genuinely unshared. + TrainingLog + .merge(manager.readWorkouts(since), publishedWorkoutsOf(myPubkey, since.epochSecond)) + .filter { it.origin == WorkoutOrigin.HEALTH_CONNECT && !it.alreadyPublished } + .sortedByDescending { it.startTimeEpochSeconds } } else { emptyList() } diff --git a/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/fitness/TrainingLog.kt b/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/fitness/TrainingLog.kt index ef62be0f78..d6cb6c6d08 100644 --- a/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/fitness/TrainingLog.kt +++ b/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/fitness/TrainingLog.kt @@ -94,7 +94,10 @@ object TrainingLog { fun WorkoutRecordEvent.toDetectedWorkout(): DetectedWorkout? { val activity = activityType() ?: return null - val duration = durationSeconds() ?: return null + // effectiveDurationSeconds, not durationSeconds: a POWR-dialect record carries `start`/`end` + // and no `duration` tag, so the raw accessor returns null and the workout would vanish from + // the log entirely. WorkoutDisplay already reads it through the same helper. + val duration = effectiveDurationSeconds() ?: return null if (duration <= 0) return null return DetectedWorkout( diff --git a/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/fitness/PublishedWorkoutMappingTest.kt b/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/fitness/PublishedWorkoutMappingTest.kt index 264e4735cf..3664f09399 100644 --- a/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/fitness/PublishedWorkoutMappingTest.kt +++ b/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/fitness/PublishedWorkoutMappingTest.kt @@ -30,6 +30,8 @@ import com.vitorpamplona.quartz.experimental.fitness.workout.tags.ExerciseTag import com.vitorpamplona.quartz.experimental.fitness.workout.tags.ExerciseType import com.vitorpamplona.quartz.experimental.fitness.workout.tags.StepsTag import com.vitorpamplona.quartz.experimental.fitness.workout.tags.TitleTag +import com.vitorpamplona.quartz.experimental.fitness.workout.tags.WorkoutEndTag +import com.vitorpamplona.quartz.experimental.fitness.workout.tags.WorkoutStartTag import com.vitorpamplona.quartz.experimental.fitness.workout.tags.WorkoutStartTimeTag import org.junit.Assert.assertEquals import org.junit.Assert.assertNotNull @@ -123,6 +125,24 @@ class PublishedWorkoutMappingTest { assertNull(event(DurationTag.assemble(1800)).toDetectedWorkout()) } + /** + * POWR-dialect records carry `start`/`end` and no `duration` tag. Reading the raw duration + * accessor drops them from the log entirely. + */ + @Test + fun `a POWR record with start and end but no duration tag is kept`() { + val workout = + event( + ExerciseTag.assemble(ExerciseType.CYCLING), + WorkoutStartTag.assemble(createdAt - 3600), + WorkoutEndTag.assemble(createdAt - 387), + ).toDetectedWorkout() + + assertNotNull(workout) + assertEquals(3213, workout!!.durationSeconds) + assertEquals(createdAt - 3600, workout.startTimeEpochSeconds) + } + @Test fun `an event with no duration is skipped`() { assertNull(event(ExerciseTag.assemble(ExerciseType.RUNNING)).toDetectedWorkout()) diff --git a/docs/health-connect-play-declaration.md b/docs/health-connect-play-declaration.md index a3add5e9e7..cafe931432 100644 --- a/docs/health-connect-play-declaration.md +++ b/docs/health-connect-play-declaration.md @@ -33,9 +33,9 @@ rather than *being* the feature. Related code: -- `service/workouts/health/WorkoutStats.kt` — all dashboard arithmetic; pure, unit-tested. -- `service/workouts/health/TrainingLog.kt` — merges Health Connect with the user's own published - workouts, deduping the ones that appear in both. +- `commons/.../commons/fitness/WorkoutStats.kt` — all dashboard arithmetic; pure, unit-tested. +- `commons/.../commons/fitness/TrainingLog.kt` — merges Health Connect with the user's own + published workouts, deduping the ones that appear in both. - `ui/screen/loggedIn/workouts/fitness/MyFitnessScreen.kt` — the dashboard. - `service/workouts/health/HealthConnectManager.kt` — the only place the app touches Health Connect. - `ui/screen/loggedIn/workouts/health/HealthConnectRationaleActivity.kt` — the in-app rationale screen.