fix: code-review pass — POWR durations, pinned bottom bar, carousel duplicates

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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019egdJyBHnrATZHjs86up8f

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<DetectedWorkout> =
+    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 }
+    }

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) {

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<DetectedWorkout> {
-        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 }
-        }
-    }
 }

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<Boolean?>(null) }
     var workouts by remember { mutableStateOf<List<DetectedWorkout>>(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()
             }

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(

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())

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.