Repository navigation
chore(workouts): require steps explicitly at every WorkoutRow construction (#1444) - #1447
Merged
Merged
Conversation
…ction (#1444) `steps` was the only defaulted parameter on WorkoutRow.init, and that default is what made #1434 and #1444 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, with nothing in the read path able to tell "never had steps" from "had steps, then lost them". Dropping the default makes each of the 35 call sites state its intent, and turns the next added column into a compile error rather than another silent overwrite. The init now carries that as a rule, so the protection does not quietly lapse the next time a field is appended with a convenient default. No behaviour change: every site here builds a genuinely step-less row (imports, manual entry, the demo seeder, the CSV backfill tool, fixtures), so each passes an explicit nil. The rebuild sites that had a real value to carry were already fixed in #1434/#1445 and are untouched. Android needs no twin: Kotlin's copy() carries unmentioned fields, so a default there costs nothing. Worth noting copy() only protects a rebuild whose receiver IS the stored row, which is why the Android edit sheet still lost steps until #1445. Swift-only, no Kotlin changed. Compile-checked by swift-packages (WhoopStore, StrandImport, their tests, and the Backfill/SleepBench/SleepPSG tools) and by app-build for the app targets and StrandTests.
Re-review of this PR: making the field explicit did what it was supposed to do and turned up a site where nil is simply wrong. WorkoutSource.merge / WorkoutEditing.merge fold several sessions into one, and they aggregate every other field — durations and energy and distance are summed, avgHr is duration-weighted, maxHr is maxed, notes are joined. Steps is cumulative per session exactly like distance, but both platforms dropped it: Swift never listed it, and Kotlin's explicit construction omitted it so the parameter default supplied null. Merging two runs therefore threw away their step counts on Android and Apple alike. Now summed under the same rule energy and distance already follow: sum the present values, null when nothing carried one, never a fake zero. Mirrored fixtures on both platforms pin the sum and the all-absent case. This is the one behavioural change in the PR; every other site here genuinely builds a step-less row and passes an explicit nil. Android 4,066 tests / 0 failures.
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.
Root-cause hardening for the bug family behind @bhelm's #1434 and my #1445 — plus one real loss it surfaced.
Why
stepswas the only defaulted parameter onWorkoutRow.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.mergefold several sessions into one and aggregate every other field: durations and 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. This is the one behavioural change in the PR — a blind mechanical pass would have written
steps: nilhere and cemented the bug as intentional.Keeping the protection real
Removing one default only helps if the next field is not added with one, so the init now carries the rule itself: add a field without a default and fix the call sites the compiler points at.
Everything else is compile-time only
The remaining 34 sites genuinely build step-less rows — imports, manual entry, the demo seeder, the CSV backfill tool, fixtures — so each simply passes an explicit
nil. The rebuild sites that had a real value to carry were fixed in #1434/#1445 and are untouched.Verification
stepsTools/Backfill/Sources/backfill/main.swift, which thetools (Backfill)CI job buildsWhoopWorkoutRow()(a different type) confirmed untouchedswift-packages(WhoopStore, StrandImport, tests, tool targets) and by app-build for the app targets + StrandTests