Skip to content

Workouts: score the SAVED workout with the measured resting HR (#983) - #992

Merged
ryanbr merged 2 commits into
mainfrom
fix/saved-workout-resting-hr
Jul 31, 2026
Merged

Workouts: score the SAVED workout with the measured resting HR (#983)#992
ryanbr merged 2 commits into
mainfrom
fix/saved-workout-resting-hr

Conversation

@ryanbr

@ryanbr ryanbr commented Jul 31, 2026

Copy link
Copy Markdown
Owner

Replaces #990, cut back after two rounds of self-review each found a defect that PR had introduced.

The bug (unchanged)

The workout paths called StrainScorer.strain without a restingHR, so they silently took the hardcoded default of 60. %HRR is (bpm - resting) / (max - resting), so the default moves every zone boundary:

136 bpm, maxHR 190
  resting 60 (default)  -> 58.5% HRR -> zone 1
  resting 50 (measured) -> 61.4% HRR -> zone 2

Today's Effort and the manual rescore (#972) already thread the measured value, so a stored workout disagreed with its own re-score.

What changed from #990

This fixes only the saved workout, on both platforms. One read at save time. No new state, no lifecycle, nothing to clear.

#990 also threaded it into the live per-sample readout, which needs the value latched for the session — and that latch is where both of its bugs came from:

  1. It re-read the denominator on every sample, so a mid-session sync could re-score the whole accumulated window and visibly step the number mid-workout.
  2. With the latch in place, endWorkout cleared it before the save — so the number written to the database could differ from the one shown on screen during the session.

Neither was inherited. Both were introduced by that PR, in the same small area: the lifecycle of a value it added. The underlying one-line idea was never the risky part; deciding when to capture and release it was. Two passes, two bugs, and I would not have claimed a third pass would find nothing — hence cutting the risky half rather than iterating on it again.

The trade-off, stated plainly

The live readout stays on the default, so it can now differ from the saved figure at the end of a session. That is accepted deliberately:

  • the stored number is what history compares and what the rescore path already agrees with — that is the disagreement worth removing;
  • the live figure is an explicitly transient running estimate;
  • and a per-sample denominator has now been got wrong twice, in code that no test covers and that no CI compiles on the Swift side.

Fixing the live path properly is still worth doing — most likely by hanging the value off the ActiveWorkout object so it lives and dies with the session rather than in a separate latch. That is a different change with its own design, and it should not ride along with a one-line correctness fix.

Also unchanged from #990

  • Does not close No effort at all on a day is hard to believe, HR always does something #983. The brisk-walk zero is the Edwards 50%-HRR floor, a model question, left open with the numbers.
  • The two historical backfill paths still use 60 — they score rows from arbitrary past days, where today's resting HR is simply a different wrong denominator.
  • Effort moves in both directions: anyone whose measured resting is above 60 will see saved workout Effort go down. Worth a release-note line.
  • Workout history becomes mixed: new saves use the measured value, stored rows keep 60.
  • Only NEW workouts change; stored rows are untouched.

Verification

  • Parity test pair (carried over unchanged) pinning that resting HR moves the zone a sample lands in and that it carries through to the score. Runs in CI on both platforms.
  • Android compiles with no new errors — AppViewModel.kt at 18 before and after, all pre-existing kotlinx-coroutines classpath cascade.
  • swiftc -parse clean; doc_comment_lint and i18n_audit --ci both 0.
  • AppModel.swift is app-target Swift no CI compiles; the idiom is verified against TodayView.swift:679/:3349, which is reading, not a compiler. The change there is now two lines with no state, which is about as small as that residual risk gets.

Replaces #990, cut back after two rounds of self-review each found a defect
that PR had introduced.

The bug is unchanged: the workout paths called StrainScorer.strain without a
restingHR, so they silently took the hardcoded default of 60. %HRR is
(bpm - resting) / (max - resting), so the default moves every zone boundary —
at 136 bpm with maxHR 190 it is the difference between zone 1 and zone 2.
Today's Effort and the manual rescore (#972) already thread the measured value,
so a stored workout disagreed with its own re-score.

This version fixes ONLY the saved workout, on both platforms: one read at save
time, no new state, no lifecycle.

#990 also threaded it into the live per-sample readout, which needs the value
latched for the session — and that latch is where both of its bugs came from.
First it re-read the denominator on every sample, so a mid-session sync could
re-score the whole window and visibly step the number. Then, with the latch in
place, endWorkout cleared it before the save, so the STORED number could differ
from the one shown on screen. Neither was inherited; both were introduced by
that PR.

The live readout is a transient running estimate and stays on the default here.
That means it can now differ from the saved figure at the end of a session —
accepted deliberately, because the stored number is what history compares and
what the rescore path already agrees with, and because a per-sample denominator
has now been got wrong twice.

Tests carried over from #990 unchanged: a parity pair pinning that resting HR
moves the zone a sample lands in, and that it carries through to the score.
#983)

Re-review catch. The fix was incomplete: two lines below the strain call,
estimateBoutCalories was still being passed null/nil for resting HR — the same
defect, in the same function, with the correct value already computed and in
scope one line above.

It matters for the same reason: the calories model's active-vs-resting
threshold sits at resting + 30% HRR, so the hardcoded default silently shifts
what counts as active. And #972 already threads it in the rescore path, so
leaving it null here meant a saved workout's kcal disagreed with its own
re-score exactly as its Effort did — this PR would have fixed one of the two
and left the other, side by side.

One argument per platform.
@ryanbr

ryanbr commented Jul 31, 2026

Copy link
Copy Markdown
Owner Author

Re-reviewed. One finding — the fix was incomplete, now corrected. No defects in what was already here.

Two lines below the strain call, estimateBoutCalories was still being passed null/nil for resting HR — the same defect, in the same function, with the correct value already computed and sitting in scope one line above.

It matters for the same reason the strain did: the calories model's active-vs-resting threshold sits at resting + 30% HRR, so the hardcoded default silently shifts what counts as active. And #972 already threads it in the rescore path — so this PR as written would have made a saved workout's Effort agree with its re-score while leaving its kcal disagreeing, the two computed side by side from the same window. One argument per platform.

Also checked and clear:

  • No shadowing. The restingHR local I introduced is the only declaration of that name in either file, so it cannot be masking an outer value.
  • No lifecycle. This is the whole reason for the cut-back: one read at save time, no state, nothing to clear, no ordering. Neither of Workouts: score with the measured resting HR, not the default 60 (#983) #990's two failure modes has anywhere to live here.
  • Android compiles with no new errors (AppViewModel.kt at 18 before and after — all pre-existing kotlinx-coroutines classpath cascade); swiftc -parse clean; doc-comment lint and i18n audit both 0.

Worth noting where this one came from: it was an omission rather than something introduced, and it was found by reading outward from the changed line rather than at the line itself. The three findings across #990 and this PR have all been in that ring — adjacent code that shares the value, not the edit itself.

Still +87/-4 with 58 of those lines tests, and the residual risk is unchanged: AppModel.swift is app-target Swift no CI compiles.

@ryanbr
ryanbr merged commit 5d4828d into main Jul 31, 2026
13 checks passed
@ryanbr
ryanbr deleted the fix/saved-workout-resting-hr branch July 31, 2026 01:43
ryanbr added a commit that referenced this pull request Jul 31, 2026
….2.2/304 (#998)

main has been on 9.2.1 for 42 commits. That was invisible until the testing
build cut from main stamped 9.2.1 / versionCode 303, and Android refused to
install it over the 29 July staging build (9.2.2 / versionCode 304) — a
downgrade, surfaced as the unhelpful "App not installed".

versionCode goes to 305 rather than 304 because 304 is already published on the
rolling testing-latest tag; reusing it would leave two different builds sharing
a code.

MARKETING_VERSION and versionName move together per CLAUDE.md. No release notes
here — this is the version bump only, and 9.2.2's notes still owe users the
user-visible changes from the last 42 commits (Effort shifts from #963 and
#992, the new Apple Health permission prompt, the Oura resting-HR heal that
needs a re-sync, and the sleep-staging change from #987).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

No effort at all on a day is hard to believe, HR always does something

1 participant