Repository navigation
fix(workouts): carry per-session steps through every row rebuild (#1444) - #1445
Merged
Merged
Conversation
#1434 fixed one site where a rebuilt WorkoutRow dropped `steps`. Auditing the rest of the tree found the same defect at five more Swift sites and, contrary to what #1444 first recorded, one on Android too. The root cause is the same on both platforms: `steps` (#1058) is a trailing parameter defaulted to nil/null, so every construction site that predates 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 row field by field, so each of these dropped it: * WorkoutSource.preservingCaptured — the function whose whole job is carrying the fields the edit sheet does not expose. Renaming a workout's sport wiped its steps. * Repository.relabelDetected — re-labelling a detected bout to manual, then upserting. * Repository's post-reduction reassembly — the displayed row lost steps even when stored. * IntelligenceEngine's manual rescore — rebuilt then upserted. * WorkoutsView.asManualCopy — "Duplicate as manual" dropped it from the copy. Android is NOT immune, which the issue got wrong. Kotlin's `copy()` protects a field only when the row being copied IS the stored one — true of backfillWorkoutFromDetectedBout and relabelDetected, which were always correct. But WorkoutEditing.preservingCaptured copies the SHEET-built row, and buildManualRow has no steps input, so copy() faithfully carried a null and the edit wiped the stored value exactly as Swift did. Mirrored regression tests on both platforms pin that an edit which cannot supply steps still comes back carrying the stored value. The Kotlin test runs in default CI; the Swift twin is app-target and is compiled, not executed, by the app-build gate. Android 4,065 tests / 0 failures; i18n and doc-comment gates clean.
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.
Closes #1444. Follow-up to @bhelm's #1434, which fixed the first instance of this.
The defect
steps(#1058) is a trailing parameter defaulted to nil/null on both platforms, so every rowconstruction site that predates it kept compiling unchanged and silently began writing nothing. Nothing
in the read path can distinguish "never had steps" from "had steps, then lost them".
Swift — five sites
Swift rebuilds a
WorkoutRowfield by field, so an unlisted field is quietly defaulted:WorkoutSource.preservingCapturedRepository.relabelDetectedRepositorypost-reduction reassemblyIntelligenceEnginemanual rescoreWorkoutsView.asManualCopyAndroid is NOT immune — #1444 got this wrong
I filed #1444 claiming this was a Swift-only family because the Kotlin twins use
copy(). That is onlyhalf true, and worth stating precisely:
copy()protects a field only when the row being copied ISthe stored one. That holds for
backfillWorkoutFromDetectedBoutandrelabelDetected, which werealways correct.
It does not hold for
WorkoutEditing.preservingCaptured, which copies the sheet-built row —buildManualRowhas no steps input, socopy()faithfully carried a null and the edit wiped the storedvalue exactly as Swift did. Same user-visible bug, reached by a different route.
Tests
Mirrored regression tests on both platforms 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 the app-build gate
compiles it rather than executing it.
Verification
--rerun-tasks, new test confirmed present by name)not alter any value that survived