Repository navigation
fix(workouts): preserve stored steps during backfill - #1434
Merged
Merged
Conversation
Owner
|
Reviewed and verified — thanks @bhelm. Confirmed the mechanism: Swift rebuilds the row field by field so an unlisted field is silently defaulted, while the Kotlin twin uses Auditing the rest of the Swift tree turned up the same defect at several other rebuild sites, two of which persist the loss — most notably the manual-workout rescore at No changes needed here — this PR is correct as it stands. Happy to merge it as-is and take the remaining sites in #1444, or if you would rather extend this PR to cover them, that works too. Let me know which you prefer. |
ryanbr
added a commit
that referenced
this pull request
Aug 19, 2026
Follow-up to @bhelm's #1434, which fixed the first instance of this. `steps` (#1058) is a trailing parameter defaulted to nil/null on both platforms, so every row construction site predating it kept compiling unchanged and silently began writing nothing. Nothing in the read path can tell 'never had steps' from 'had steps, then lost them'. Swift rebuilds a WorkoutRow field by field, so five sites dropped it: WorkoutSource.preservingCaptured (the function whose whole job is carrying fields the edit sheet does not expose — renaming a workout's sport wiped its steps), Repository.relabelDetected, Repository's post-reduction display reassembly, IntelligenceEngine's manual rescore, and WorkoutsView.asManualCopy. The last two of those persist, and in Swift the duplicate-as-manual chain needs BOTH the copy and preservingCaptured fixes, since the copy becomes the sheet's `editing` row that preservingCaptured then reads. #1444 recorded this as a Swift-only family because the Kotlin twins use copy(). That is only half true: copy() protects a field ONLY when the row being copied IS the stored one. That holds for backfillWorkoutFromDetectedBout, relabelDetected, the display reconcile and duplicate-as-manual, all verified correct. It does not hold for WorkoutEditing.preservingCaptured, which copies the SHEET-built row — buildManualRow has no steps input, so copy() faithfully carried a null and the edit wiped the stored value exactly as Swift did. Android is fixed too. The override is safe in both directions: the sole caller on each platform builds its base row through buildManualRow, which cannot supply steps, so carrying old.steps can never discard a real value. Mirrored regression tests pin that an edit which cannot supply steps still returns the stored value. The Kotlin test runs in default CI; the Swift twin is app-target, so app-build compiles rather than executes it. No schema, migration or stored-format change — this stops a field being overwritten with null and alters no value that survived. Android 4,065 tests / 0 failures; i18n and doc-comment gates clean; app-build green on both legs.
ryanbr
added a commit
that referenced
this pull request
Aug 19, 2026
…ction (#1444) Root-cause hardening for the bug family behind @bhelm's #1434 and my #1445 — plus one real loss it surfaced. `steps` was the only defaulted parameter on WorkoutRow.init, and that default is what made both bugs silent. Swift rebuilds a row field by field, so when the column was added (#1058) every construction site that predated it kept compiling unchanged and quietly began writing nothing, at six sites, two of which persisted the loss. Nothing in the read path can tell 'never had steps' from 'had steps, then lost them', so it never surfaced as a symptom. Dropping the default makes all 35 call sites state their intent and turns the next added column into a compile error instead of another silent overwrite. It immediately caught a seventh site, on BOTH platforms. WorkoutSource.merge / WorkoutEditing.merge fold several sessions into one and aggregate every other field: durations, energy and distance summed, avgHr duration-weighted, maxHr maxed, notes joined. Steps is cumulative per session exactly like distance, and both platforms dropped it — Swift never listed it, Kotlin's explicit construction omitted it so the parameter default supplied null. Merging two runs threw away their step counts on Android and Apple alike. It is now summed under the same rule energy and distance already follow: sum the present values, nil when nothing carried one, never a fake zero. A blind mechanical pass would have written `steps: nil` here and cemented the bug as intentional. Removing one default only helps if the next field is not added with one, so the init carries that rule itself. Every other site here genuinely builds a step-less row and passes an explicit nil; the rebuild sites with a real value to carry were fixed in #1434/#1445. Repo-wide rescan leaves 0 constructions without an explicit steps, including one outside the app and package trees that a first pass missed (Tools/Backfill, which the tools (Backfill) CI job builds). Mirrored merge fixtures pin the sum and the all-absent case. Android 4,066 tests / 0 failures. Swift-only; no Kotlin file changed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Preserves a workout's stored
stepswhen Swift reconstructs the row while backfilling missing heart-rate, energy, or strain fields.Swift previously omitted
stepsfrom the reconstructed row, resetting it tonil. Carrying the field forward matches Kotlin's existingcopybehavior. Regression tests cover both a partially filled row and an already complete row.Type of change
How it was tested
cd Packages/StrandAnalytics && swift test --filter WorkoutDetectorBackfillTests— 3 tests passedOnly the focused suite was run. No manual, device, or strap testing was performed; the preservation behavior is covered by a deterministic unit fixture and matches the existing Kotlin implementation.
Checklist
swift testinPackages/<name>) — focusedWorkoutDetectorBackfillTestspassed; the full package suite was not runandroid/(./gradlew testFullDebugUnitTest) — not applicable; Android was not changedStrandDesigntokens — not applicable; no UI changeddocs/CONTRIBUTING.mdStrand.xcodeproj/) or any secrets/keystoresRelated issues
Related: bhelm#16