Merge upstream ryanbr/noop main into the fork (through 137dc0bfe)

Packages/StrandDesign/Sources/StrandDesign/MotionTrace.swift

@@ -33,12 +33,27 @@ public struct MotionTrace: View {
 
     /// The peak magnitude used to normalise the fill height. A non-positive peak (all-zero / empty) maps
     /// everything to the baseline so the strip is flat rather than dividing by zero.
-    private var peak: Double { max(epochs.max() ?? 0, 0) }
+    ///
+    /// Computed ONCE where it is needed and threaded down, never read inside a per-epoch loop. As a
+    /// computed property it rescanned every epoch on each read, and it was read from inside the `map` in
+    /// `points(in:)` and the `filter` in `accessibilitySummary`, so a night cost a scan per epoch. With
+    /// 30-second epochs an 8-hour night is ~960 of them, about 1.8 million comparisons every time the
+    /// strip is laid out, and SwiftUI re-runs `body` on hover, animation and the 1 Hz HR tick (#2283).
+    ///
+    /// Android already hoists the same value (`SleepScreen.kt`), so this removes a divergence rather than
+    /// creating one.
+    static func peak(of epochs: [Double]) -> Double { max(epochs.max() ?? 0, 0) }
 
     public var body: some View {
         GeometryReader { geo in
             let w = geo.size.width
             let h = geo.size.height
+            // ONE scan, threaded into everything below, rather than a scan per epoch. See `peak(of:)`.
+            //
+            // Sits inside the GeometryReader, so it is recomputed per LAYOUT pass rather than per body
+            // evaluation. That distinction does not matter here (both are O(n) against the O(n^2) this
+            // replaces) and keeping it here avoids restructuring `body` around the ViewBuilder.
+            let peak = Self.peak(of: epochs)
             ZStack {
                 // Faint baseline so the strip reads as a grounded trace even on a calm night.
                 Path { p in
@@ -49,7 +64,7 @@ public struct MotionTrace: View {
 
                 // Filled area under the per-epoch magnitude, normalised to the night's own peak.
                 if epochs.count >= 2, peak > 0 {
-                    let pts = points(in: geo.size)
+                    let pts = Self.points(in: geo.size, epochs: epochs, peak: peak)
                     Path { p in
                         p.move(to: CGPoint(x: 0, y: h))
                         for pt in pts { p.addLine(to: pt) }
@@ -77,14 +92,14 @@ public struct MotionTrace: View {
             }
             .accessibilityElement()
             .accessibilityLabel(Text("Movement during sleep"))
-            .accessibilityValue(Text(accessibilitySummary))
+            .accessibilityValue(Text(Self.accessibilitySummary(epochs: epochs, peak: peak)))
         }
         .frame(height: height)
     }
 
     /// One screen point per epoch: x spread evenly across the width (matching the hypnogram's left→right
     /// time mapping), y the magnitude normalised to the night's peak (0 at the baseline, full at the top).
-    private func points(in size: CGSize) -> [CGPoint] {
+    static func points(in size: CGSize, epochs: [Double], peak: Double) -> [CGPoint] {
         let n = epochs.count
         guard n >= 2, peak > 0 else { return [] }
         let h = size.height
@@ -98,7 +113,7 @@ public struct MotionTrace: View {
 
     /// A coarse VoiceOver summary — the share of epochs with above-half-peak movement — since a per-epoch
     /// trace can't be voiced point by point. "Calm" when nothing crosses the threshold.
-    private var accessibilitySummary: String {
+    static func accessibilitySummary(epochs: [Double], peak: Double) -> String {
         guard peak > 0, !epochs.isEmpty else { return "no movement data" }
         let restless = epochs.filter { $0 >= peak * 0.5 }.count
         if restless == 0 { return "calm throughout" }

Packages/StrandDesign/Tests/StrandDesignTests/MotionTracePeakTests.swift

@@ -0,0 +1,97 @@
+import XCTest
+import CoreGraphics
+@testable import StrandDesign
+
+/// Hoisting `peak` out of the per-epoch loops must not change a single pixel or a single word (#2283).
+///
+/// `peak` was a computed property that rescanned every epoch, read from inside the `map` in `points` and
+/// the `filter` in `accessibilitySummary`. That is a scan per epoch: with 30-second epochs an 8-hour
+/// night is ~960 of them, about 1.8 million comparisons every time the strip is laid out, repeated
+/// because SwiftUI re-runs `body` on hover, animation and the 1 Hz HR tick.
+///
+/// These pin the OUTPUT rather than the speed. A performance change that alters what is drawn is not a
+/// performance change, it is a regression, and the normalisation, the half-peak threshold and the
+/// degenerate cases are exactly where a careless hoist would show it.
+final class MotionTracePeakTests: XCTestCase {
+
+    private let size = CGSize(width: 100, height: 40)
+
+    /// The pre-hoist definitions, transcribed, so the new code is compared against the old behaviour
+    /// rather than against itself.
+    private func referencePoints(_ epochs: [Double]) -> [CGPoint] {
+        let peak = max(epochs.max() ?? 0, 0)
+        let n = epochs.count
+        guard n >= 2, peak > 0 else { return [] }
+        let h = size.height
+        let usable = h - 2
+        return epochs.enumerated().map { i, v in
+            let x = CGFloat(i) / CGFloat(n - 1) * size.width
+            let frac = CGFloat(max(0, min(v / peak, 1)))
+            return CGPoint(x: x, y: h - frac * usable)
+        }
+    }
+
+    private func referenceSummary(_ epochs: [Double]) -> String {
+        let peak = max(epochs.max() ?? 0, 0)
+        guard peak > 0, !epochs.isEmpty else { return "no movement data" }
+        let restless = epochs.filter { $0 >= peak * 0.5 }.count
+        if restless == 0 { return "calm throughout" }
+        let pct = Int((Double(restless) / Double(epochs.count) * 100).rounded())
+        return "\(pct)% of the night had elevated movement"
+    }
+
+    private func assertMatches(_ epochs: [Double], _ label: String,
+                               file: StaticString = #filePath, line: UInt = #line) {
+        let peak = MotionTrace.peak(of: epochs)
+        XCTAssertEqual(MotionTrace.points(in: size, epochs: epochs, peak: peak),
+                       referencePoints(epochs), label, file: file, line: line)
+        XCTAssertEqual(MotionTrace.accessibilitySummary(epochs: epochs, peak: peak),
+                       referenceSummary(epochs), label, file: file, line: line)
+    }
+
+    func testAnOrdinaryNightIsUnchanged() {
+        // 30-second epochs across eight hours, the real shape this draws.
+        let epochs = (0..<960).map { i in Double((i * 37) % 100) / 10.0 }
+        assertMatches(epochs, "ordinary night")
+    }
+
+    func testDegenerateNightsAreUnchanged() {
+        // The cases where a careless hoist changes behaviour: dividing by a zero peak, or losing the
+        // guard that keeps an empty strip flat rather than crashing.
+        assertMatches([], "empty")
+        assertMatches([0], "single zero epoch")
+        assertMatches([4.2], "single non-zero epoch")
+        assertMatches([0, 0, 0, 0], "all zero")
+        assertMatches([-1, -2], "negative magnitudes clamp to a flat strip")
+    }
+
+    func testHalfPeakThresholdIsUnchanged() {
+        // `accessibilitySummary` counts epochs at or above half the peak, so a value exactly on the
+        // boundary is the one that would move if the peak were computed differently.
+        assertMatches([10, 5, 4.999, 0], "values straddling half peak")
+        assertMatches([1, 1, 1], "every epoch at the peak")
+    }
+
+    func testTheStripsOwnHelpersAgreeOnTheSamePeak() {
+        // The gap these tests cannot close: nothing here checks that `body` passes the peak it computed
+        // into both helpers. Pin the next best thing, that the helpers agree when handed the peak the
+        // hoisted accessor produces, so a caller threading a DIFFERENT value is the only way to break it.
+        let epochs: [Double] = [0, 3, 9, 4.5, 4.4, 0]
+        let peak = MotionTrace.peak(of: epochs)
+        XCTAssertEqual(peak, 9)
+        let pts = MotionTrace.points(in: size, epochs: epochs, peak: peak)
+        XCTAssertEqual(pts.count, epochs.count)
+        // 9 is the peak, so it must land at the very top of the usable band, and 0 at the baseline.
+        XCTAssertEqual(pts[2].y, size.height - (size.height - 2), accuracy: 0.0001)
+        XCTAssertEqual(pts[0].y, size.height, accuracy: 0.0001)
+        // 4.5 is exactly half the peak and counts as restless; 4.4 does not. Two of six is 33%.
+        XCTAssertEqual(MotionTrace.accessibilitySummary(epochs: epochs, peak: peak),
+                       "33% of the night had elevated movement")
+    }
+
+    func testPeakIgnoresNegativesAndEmpties() {
+        XCTAssertEqual(MotionTrace.peak(of: []), 0)
+        XCTAssertEqual(MotionTrace.peak(of: [-5, -1]), 0, "a negative peak clamps to zero")
+        XCTAssertEqual(MotionTrace.peak(of: [1, 9, 3]), 9)
+    }
+}

Strand/App/AppModel.swift

@@ -1082,6 +1082,18 @@ final class AppModel: ObservableObject {
     /// Finish the active workout: finalize the GPS route (#524), score the captured HR window, and save it
     /// as a `WorkoutRow`. A session with no HR window AND no real GPS route is discarded quietly (parity
     /// with Android) , but a GPS-only walk with HR not streaming still saves. Double-buzz confirms.
+    /// Shortest live session worth keeping. Below this a start/stop is an accident, not training (#2278).
+    static let minimumWorkoutSeconds: TimeInterval = 60
+
+    /// Whether a finished live session is too short to save.
+    ///
+    /// A named predicate rather than an inline comparison so the boundary is pinned by a test and so the
+    /// Android twin has one thing to mirror. Exactly `minimumWorkoutSeconds` is KEPT: a wearer who logs a
+    /// deliberate one-minute effort gets to keep it, and the discard is for what falls short of that.
+    nonisolated static func isTooShortToSave(elapsedSeconds: TimeInterval) -> Bool {
+        elapsedSeconds < minimumWorkoutSeconds
+    }
+
     func endWorkout() {
         guard let w = activeWorkout else { return }
         endZoneTraining()
@@ -1113,6 +1125,26 @@ final class AppModel: ObservableObject {
             return
         }
         let end = Date()
+        // A session under a minute is a start/stop the wearer did not mean to keep, and it was the thing
+        // that made deletion feel broken: the list filled with 5-30 second entries (#2278). Discarded HERE,
+        // at save, rather than retained and pruned later, which is the whole difference between dropping
+        // something that never had training data in it and deleting a wearer's history. NOOP has no server
+        // and no cloud copy, so a later prune would be irreversible; this is not, because nothing with real
+        // data is ever removed.
+        //
+        // Sits after the sample/route gate above so that gate's meaning is unchanged: a 30-second session
+        // can easily carry two HR samples and would otherwise have been saved.
+        let elapsed = w.elapsed(at: end)
+        if Self.isTooShortToSave(elapsedSeconds: elapsed) {
+            emitWorkoutsTrace(WorkoutsTrace.sessionLine(
+                event: "discarded", sportKey: WorkoutSource.traceSportKey(w.sport),
+                hrSamples: samples.count, durationSec: Int(elapsed),
+                gpsPoints: wasGps ? gpsRecorder.pointCount : nil))
+            // Drop the route too: keeping a polyline for a session that was never saved would orphan it in
+            // RouteStore under a natural key no row claims.
+            lastWorkout = nil
+            return
+        }
         let avg = samples.isEmpty ? nil
             : Int((Double(samples.map(\.bpm).reduce(0, +)) / Double(samples.count)).rounded())
         let peak = samples.map(\.bpm).max()

Strand/Data/Repository.swift

@@ -362,6 +362,43 @@ final class Repository: ObservableObject {
         return Self.rawWhoopSourceIds(activeDeviceId: deviceId, registeredWhoopIds: registeredWhoopIds)
     }
 
+    /// Every namespace the Workouts list reads a row out of, in read order, duplicates collapsed.
+    ///
+    /// Why this exists: the list unions many device ids while `deleteWorkout` deleted from exactly ONE,
+    /// the active strap. A row banked under any other namespace (a retained strap, a computed `-noop`
+    /// sibling, Apple Health, an imported lifting session or activity file) was therefore VISIBLE BUT
+    /// UNDELETABLE: the delete reported nothing, the reload re-read the row from the namespace the delete
+    /// never touched, and it came straight back. Reported as workouts that ignore the delete button
+    /// (#2278).
+    ///
+    /// Deriving both sides from one function is the point. Spelling the union out twice is what let them
+    /// disagree, and a future namespace added to the read alone would reintroduce exactly this bug.
+    nonisolated static func workoutNamespaces(rawIds: [String]) -> [String] {
+        deletableWorkoutNamespaces(rawIds: rawIds)
+            + [WorkoutSource.appleHealthSource, "lifting", HevySource.id, "activity-file"]
+    }
+
+    /// The subset of [workoutNamespaces] a DELETE may touch: the strap namespaces only.
+    ///
+    /// Imported history is read-only, and that is enforced everywhere else: the row menu offers only
+    /// "Duplicate as manual…" for an imported row, `bulkDeleteWorkouts` skips those classes outright, and
+    /// `mergeWorkouts` refuses them with "never rewrite imported history". A delete that swept the import
+    /// namespaces would reach underneath all three guards and destroy a wearer's imported Apple Health,
+    /// Hevy/Liftosaur or FIT/GPX/TCX row, which nothing in the UI ever offers to remove.
+    ///
+    /// That a cross-source twin is COLLAPSED into one row at display time does not license deleting the
+    /// imported half of the pair: the dedup is a presentation decision, and the surviving import is
+    /// exactly the history this repository promises not to rewrite.
+    ///
+    /// A `.manual` row, the only class the delete button is offered for, is written under a strap id, so
+    /// this set is what a delete actually needs.
+    nonisolated static func deletableWorkoutNamespaces(rawIds: [String]) -> [String] {
+        (rawIds + rawIds.map { $0.hasSuffix("-noop") ? $0 : $0 + "-noop" })
+            .reduce(into: [String]()) { acc, id in
+                if !acc.contains(id) { acc.append(id) }
+            }
+    }
+
     /// Pure ordering contract shared with Android's parity guard: current active source first, every other
     /// registered WHOOP in stable registry order, canonical history last; duplicates collapse.
     nonisolated static func rawWhoopSourceIds(activeDeviceId: String,
@@ -3433,25 +3470,18 @@ final class Repository: ObservableObject {
         // De-dup identical same-source rows that appear under both union ids by natural key (the cross-SOURCE
         // dedup below only collapses strap-vs-Apple twins, not a row present in two strap namespaces).
         var rows: [WorkoutRow] = []
-        let rawIds = rawPhysiologyReadIds(store: store)
-        for id in rawIds { rows += await pagedWorkoutRows(store: store, deviceId: id, from: lo, to: hi) }
-        for id in rawIds.map({ $0.hasSuffix("-noop") ? $0 : $0 + "-noop" }) {
+        // Every namespace in one list, shared with `deleteWorkout` so the two cannot disagree about where a
+        // row lives (#2278). File-imported lifting sessions (Hevy / Liftosaur exports) live under "lifting";
+        // API-synced Hevy sessions deliberately use their own source so an export and its API twin can be
+        // fused instead of one silently overwriting the other. #29: imported activity FILES (FIT / GPX /
+        // TCX) live under "activity-file", or a successful file import never appears here at all.
+        for id in Self.workoutNamespaces(rawIds: rawPhysiologyReadIds(store: store)) {
             rows += await pagedWorkoutRows(store: store, deviceId: id, from: lo, to: hi)
         }
-        rows += await pagedWorkoutRows(store: store, deviceId: "apple-health", from: lo, to: hi)
-        // File-imported lifting sessions (Hevy / Liftosaur exports) live under "lifting". API-synced
-        // Hevy sessions deliberately use their own source so an export and its API twin can be fused
-        // instead of one silently overwriting the other.
-        rows += await pagedWorkoutRows(store: store, deviceId: "lifting", from: lo, to: hi)
-        rows += await pagedWorkoutRows(store: store, deviceId: "hevy", from: lo, to: hi)
         // Native sessions remain normalized in the training tables. Their read-time envelope makes
         // them visible to Workouts and session fusion without persisting a duplicate workout row.
         let native = await pagedNativeWorkoutRows(store: store, from: lo, to: hi)
         rows += native.map(NativeTrainingProjection.workoutRow)
-        // #29: imported activity FILES (FIT / GPX / TCX) live under their own "activity-file" source — read
-        // them too, or a successful file import never appears in the Workouts list (Data Sources counts it,
-        // the load didn't). HR is reconciled from the strap trace at the end like every other row.
-        rows += await pagedWorkoutRows(store: store, deviceId: "activity-file", from: lo, to: hi)
         rows = Self.dedupWorkoutsByNaturalKey(rows)
         let lifecycleLinks = ((try? await store.trainingSessionLinks()) ?? []).filter { $0.origin == "native-lifecycle" }
         rows = Self.hidingLegacyStrengthRecordings(rows, links: lifecycleLinks)
@@ -3784,8 +3814,22 @@ final class Repository: ObservableObject {
     func deleteWorkout(_ row: WorkoutRow) async {
         if WorkoutSource.classify(row.source) == .detected { await dismissDetected(row); return }
         guard let store = await ensureStore() else { return }
-        _ = try? await store.deleteWorkouts(deviceId: deviceId, sport: row.sport,
-                                            from: row.startTs, to: row.startTs)
+        // Sweep every STRAP namespace, not just the active one. A manual row banked under a retained
+        // strap or a computed sibling is shown by `workoutRows` and was previously undeletable: the
+        // delete touched one namespace, the reload re-read the row from another, and it reappeared
+        // (#2278).
+        //
+        // Import namespaces are deliberately excluded, see `deletableWorkoutNamespaces`: imported
+        // history is read-only and no UI offers to remove it.
+        //
+        // Narrow by construction. The natural key is exact (`sport` plus a single `startTs`), so this
+        // removes the row the wearer tapped and its copies in the strap namespaces, nothing else. An
+        // overlapping-but-differently-keyed session is NOT touched; collapsing those is the dedup's job
+        // at display time, not a delete's.
+        for id in Self.deletableWorkoutNamespaces(rawIds: rawPhysiologyReadIds(store: store)) {
+            _ = try? await store.deleteWorkouts(deviceId: id, sport: row.sport,
+                                                from: row.startTs, to: row.startTs)
+        }
     }
 
     /// #64: merge two-or-more overlapping / adjacent MANUAL or DETECTED sessions into ONE manual session