rescale strain to the load actually earned, and backfill what is stored - #240
Open
svssathvik7 wants to merge 1 commit into
Open
rescale strain to the load actually earned, and backfill what is stored#240svssathvik7 wants to merge 1 commit into
svssathvik7 wants to merge 1 commit into
Conversation
Rides the analytics change that redefines the 0-21 headline as the TRIMP earned
ABOVE a quiet-waking baseline rather than whole-waking-day TRIMP through a
log base 1.5. On a real bundle an INACTIVE full-wear day scored 12.8 out of 21,
because ~16 h of simply being awake accrues ~180 TRIMP and that curve is
steepest near zero. 21 sat at TRIMP ~4987 — about 35 h at 80 % HRR — so the top
of the scale was unreachable and a marathon read ~15.8.
Both call sites now pass the wake window the TRIMP was accumulated over, since
that is what sets the baseline: `perMin.length` in the engine and the pure
pipeline, the window's own length for a manually logged session. Passing the
OBSERVED length rather than an assumed full day is what stops a partial-wear day
from being charged a whole day's overhead — a real bundle (2026-07-10) was worn
135 waking minutes. Sex is threaded through with it so the baseline is priced by
the same Banister constant that scored the TRIMP.
The intraday strain curve picks up Banister's 0.64/0.86 scale coefficient. It
had been inlining `exp(b*hrr)` and dropping the coefficient entirely, so it
accumulated a TRIMP 1.5625x the day's own — tolerable while it was only a shape,
but not once the headline nets against a baseline priced with `banisterY`. Its
baseline grows with the wake window already elapsed, so the curve stays flat
through quiet waking and climbs only on real effort.
BACKFILL. Raw 1 Hz substrate is pruned `rawRetentionDays` (3) behind the data
edge, so history cannot be re-derived from raw — the engine keeps the old row
and logs "no substrate (raw pruned)". It does not need raw: strain is a pure
function of (TRIMP, wake minutes, sex), and metric_series already stores trimp,
worn_min and tst_min for every derived day. On a real bundle `strain_curve` has
611 points against `worn_min - tst_min` = 611, so the reconstructed wake window
is the one the pipeline fed the scorer.
`strain_backfill.dart` rebuilds the headline from those, rewriting metric_series
(trends, sparklines, v_daily, the coach's SQL surface, baselines) and writing a
corrected day_result row at the new algo version — a version bump writes a NEW
row, so the immutable-per-version invariant holds. It runs once, before the
sweep, guarded by compute_freshness, and never fatally: a failed rescale must
not take the derive cycle down.
Days INSIDE the raw window are deliberately left alone for a real re-derive.
Patching them would write a row at kAlgoVersion, and the derive gate matches
algo_version exactly, so a partial patch would stand in for a full
re-derivation. A day that cannot be rescaled — no stored TRIMP, or no wake
window — is left exactly as it was and counted, because 0 is a number, not an
absence. The stale `series.strain_curve` is DROPPED on rescaled days: it was
built from per-sample HR that no longer exists, and its last point IS the old
headline, so a curve ending at 12.79 under a headline of 9.03 contradicts
itself.
SEPARATE BUG, found while tracing the target the user actually sees. There were
two strain targets with different key names and only one producer.
crossDayPipeline emits `strain_coach` ({target_min,target_max,band}) into the
insights map, which the Insights card reads. CoachData reads
`coach.strain_target` ({value,low,high}) — and NOTHING wrote a `coach` key
anywhere: `sub('coach')` in payloads.dart was the only occurrence of it in the
tree. Today's plan chip, the Coach screen's target tile and the home-screen
widget all rendered nothing, silently, while a test fixture "covered" the shape
production never emitted. `coachToday()` bridges the two and getToday() emits
it; the fixture's shape is now something the app actually produces.
📝 WalkthroughWalkthroughChangesThe strain scale changes to use quiet-waking baseline subtraction, observed wake duration, and sex-specific weighting. Historical records can be rescaled from stored metrics. Today data now includes cross-day strain coach targets. Strain scale recalibration
Coach target wiring
Estimated code review effort: 5 (Critical) | ~90+ minutes Sequence Diagram(s)sequenceDiagram
participant DerivationEngine
participant backfillStrainScale
participant LocalDb
participant strainScoreMetric
DerivationEngine->>backfillStrainScale: run historical strain backfill
backfillStrainScale->>LocalDb: load stored TRIMP and wake metrics
backfillStrainScale->>strainScoreMetric: calculate duration-aware strain
strainScoreMetric-->>backfillStrainScale: return recalculated strain
backfillStrainScale->>LocalDb: update series and day bundles
DerivationEngine->>strainScoreMetric: calculate current headline strain
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/compute/derivation_engine.dart`:
- Around line 1125-1141: The one-shot strain backfill currently runs only in the
full derive flow, so selected re-analysis through runDays() skips it. Extract
the existing guarded backfill and logging logic into a shared method, invoke
that method from both the current derive path and runDays(), and preserve its
one-shot, non-fatal behavior. Add a regression test covering selected
re-analysis of a raw-pruned legacy day.
In `@lib/compute/strain_backfill.dart`:
- Around line 95-105: Update the retention cutoff calculation in the strain
backfill flow around _markDone() to use LocalDb.lastDecodedRecTs() as the
decoded-data edge instead of days.last from strainRows. When no decoded record
exists, leave the cutoff unset so all eligible stored days are processed;
preserve existing retention-day shifting when a decoded timestamp is available,
and add coverage for decoded data newer than the newest strain row.
- Around line 111-175: Make the historical rewrite in the backfill flow atomic
by replacing the separate read/write sequence around LocalDb.dayResult and
LocalDb.putDayResult with one database transaction that rechecks absence of a
current-version row before writing the bundle and series; skip the stale rewrite
when another derivation has already produced version kAlgoVersion. Update the
compute_freshness claim within that same transaction, preserving idempotent
replacement semantics and preventing current payload details or strain_curve
data from being overwritten. Add an interleaving regression test covering a
concurrent current-version write.
- Around line 209-215: Update _shiftDays to use local calendar arithmetic by
constructing the target with DateTime(year, month, day + days) instead of adding
Duration(days: days), then return the formatted result through dayLabelOf(). Add
a test covering a spring-forward DST transition and verifying the expected date
label.
- Around line 65-69: Update the strain calculation near the existing
ana.strainScore call to use an API supported by the pinned analytics revision:
either upgrade to a revision defining the wakeMinutes and female overload, or
call strainScoreMetric(trimp) and return its value only when non-null. Preserve
the existing null and wake validation behavior.
In `@lib/data/local_repository_impl.dart`:
- Around line 3266-3275: Reject inverted strain-target bounds in the
strain-target mapping logic by returning null when lo is greater than hi, while
preserving valid ranges. Add a regression expectation for reversed numeric
bounds in test/coach_strain_target_wiring_test.dart at lines 84-87.
- Around line 362-364: Update getToday() to call _crossDay() before its
empty-response early return, and include coachToday(cd) under the 'coach' key in
that return shape. Preserve the existing coach field in the normal response and
add a regression test covering strain_coach without a daily bundle or wake
features.
In `@test/strain_rescale_backfill_test.dart`:
- Around line 136-205: Make the tests in the backfillStrainScale group
independent by resetting and seeding all required database rows in setUp(),
including both historical dates and their expected payload state. Ensure each
test can run alone or in any order, while preserving the existing assertions for
rescaling, raw-retention skipping, curve removal, idempotence, and missing-TRIMP
handling.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8c4bf436-e6c8-4732-8d54-0fa6ad2b4167
📒 Files selected for processing (8)
lib/compute/derivation_engine.dartlib/compute/manual_session.dartlib/compute/onehz_pipeline.dartlib/compute/strain_backfill.dartlib/data/db.dartlib/data/local_repository_impl.darttest/coach_strain_target_wiring_test.darttest/strain_rescale_backfill_test.dart
Comment on lines
+1125
to
+1141
| // ONE-SHOT: rescale stored strain onto the v63 scale. Days inside the raw | ||
| // window re-derive below from substrate; everything older has none, so its | ||
| // headline is rebuilt from the stored TRIMP + wake window instead. Runs | ||
| // before the sweep so the two never disagree mid-pass, and no-ops after the | ||
| // first successful pass (`compute_freshness`). Never fatal — a failed | ||
| // rescale must not take the derive cycle down with it. | ||
| try { | ||
| final rescaled = await backfillStrainScale( | ||
| female: workoutSex(profile.sex) == 'female', | ||
| ); | ||
| if (rescaled.didWork) { | ||
| _log('[derive] strain rescale: ${rescaled.bundleDays} day(s) rebuilt, ' | ||
| '${rescaled.skipped} skipped (no TRIMP or no wake window)'); | ||
| } | ||
| } catch (e) { | ||
| _log('[derive] strain rescale failed (kept old values): $e'); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline lib/compute/derivation_engine.dart --items all --type method
rg -n -C 4 'runDays\s*\(|\.runDays\s*\(|backfillStrainScale\s*\(' lib testRepository: OpenStrap/edge
Length of output: 11127
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- DerivationEngine.run and shared helpers ---'
sed -n '1000,1170p' lib/compute/derivation_engine.dart
printf '%s\n' '--- DerivationEngine.runDays and adjacent entry paths ---'
sed -n '1300,1535p' lib/compute/derivation_engine.dart
printf '%s\n' '--- strain backfill implementation ---'
sed -n '1,180p' lib/compute/strain_backfill.dart
printf '%s\n' '--- related tests and freshness references ---'
rg -n -C 8 'kStrainRescaleKey|strain rescale|runDays|rescanRecent|backfillStrainScale' lib/compute lib/state testRepository: OpenStrap/edge
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- rescanRecent body and completion path ---'
sed -n '2100,2295p' lib/compute/derivation_engine.dart
printf '%s\n' '--- runDays backfill-related calls in DerivationEngine ---'
python3 - <<'PY'
from pathlib import Path
p = Path('lib/compute/derivation_engine.dart')
s = p.read_text()
for name in ('run(', 'runDays(', 'rescanRecent('):
start = s.find('Future<int> ' + name)
if start < 0:
print(name, 'NOT FOUND')
continue
next_methods = [s.find('\n Future<int> ', start + 1), s.find('\n Future<void> ', start + 1)]
end = min(x for x in next_methods if x >= 0)
body = s[start:end]
print(f'{name}: backfillStrainScale={body.count("backfillStrainScale(")}, '
f'runDays={body.count(".runDays(")}, '
f'returns={body.count("return ")}')
PY
printf '%s\n' '--- existing selected-day strain/backfill regression coverage ---'
rg -n -i -C 5 'selected.*day|historical|raw.?prun|strain.*runDays|runDays.*strain|strain.*rescal|rescal.*runDays' test libRepository: OpenStrap/edge
Length of output: 50371
Run the strain backfill from runDays().
runDays() never calls backfillStrainScale(). A selected re-analysis of a raw-pruned legacy day therefore leaves its old strain values unchanged. Extract the one-shot gate into a shared method and add a runDays() regression test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/compute/derivation_engine.dart` around lines 1125 - 1141, The one-shot
strain backfill currently runs only in the full derive flow, so selected
re-analysis through runDays() skips it. Extract the existing guarded backfill
and logging logic into a shared method, invoke that method from both the current
derive path and runDays(), and preserve its one-shot, non-fatal behavior. Add a
regression test covering selected re-analysis of a raw-pruned legacy day.
Source: Coding guidelines
Comment on lines
+65
to
+69
| if (trimp == null || wornMin == null) return null; | ||
| final wake = wornMin - (tstMin ?? 0); | ||
| if (wake <= 0) return null; | ||
| return ana.strainScore(trimp, wakeMinutes: wake, female: female); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 'openstrap_analytics' pubspec.yaml pubspec.lock
rg -n -C 3 'strainScoreMetric\s*\(|strainScore\s*\(' lib testRepository: OpenStrap/edge
Length of output: 4447
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- strain_backfill.dart ---'
sed -n '1,90p' lib/compute/strain_backfill.dart
printf '%s\n' '--- dependency declarations ---'
sed -n '45,65p' pubspec.yaml
sed -n '955,980p' pubspec.lock
printf '%s\n' '--- local API usage ---'
rg -n -C 4 'strainScoreMetric|strainScore\s*\(' lib/compute test
printf '%s\n' '--- repository metadata ---'
git status --short
git ls-files | rg '(^|/)(pubspec\.lock|strain_backfill\.dart)$'Repository: OpenStrap/edge
Length of output: 10554
🏁 Script executed:
#!/bin/bash
set -euo pipefail
repo='OpenStrap/analytics'
sha='e047c5920ea2e0def28f028d0fc990c1fe64cd6b'
base="https://raw.githubusercontent.com/${repo}/${sha}"
printf '%s\n' '--- commit resolution ---'
curl -fsSL "https://api.github.com/repos/${repo}/commits/${sha}" | jq -r '.sha'
printf '%s\n' '--- exported onehz.dart ---'
curl -fsSL "${base}/lib/onehz.dart"
printf '%s\n' '--- files defining or declaring scorer APIs ---'
curl -fsSL "https://api.github.com/repos/${repo}/git/trees/${sha}?recursive=1" |
jq -r '.tree[] | select(.type == "blob" and (.path | endswith(".dart"))) | .path' |
while read -r path; do
content="$(curl -fsSL "${base}/${path}")"
if printf '%s\n' "$content" | rg -q 'strainScoreMetric|double\??\s+strainScore|strainScore\s*\('; then
printf '%s\n' "--- ${path} ---"
printf '%s\n' "$content" |
rg -n -C 8 'strainScoreMetric|double\??\s+strainScore|strainScore\s*\('
fi
doneRepository: OpenStrap/edge
Length of output: 229
🏁 Script executed:
#!/bin/bash
set -euo pipefail
repo_url='https://github.com/OpenStrap/analytics.git'
sha='e047c5920ea2e0def28f028d0fc990c1fe64cd6b'
base="https://raw.githubusercontent.com/OpenStrap/analytics/${sha}"
printf '%s\n' '--- pinned commit exists ---'
git ls-remote "$repo_url" "$sha"
printf '%s\n' '--- exported onehz.dart ---'
curl -fsSL "$base/lib/onehz.dart"
printf '%s\n' '--- scorer definitions in likely source files ---'
for path in lib/onehz.dart lib/src/onehz.dart lib/src/strain.dart lib/src/metrics.dart; do
if content="$(curl -fsSL "$base/$path" 2>/dev/null)"; then
printf '%s\n' "--- $path ---"
printf '%s\n' "$content" |
rg -n -C 10 'strainScoreMetric|double\??\s+strainScore|strainScore\s*\(' || true
fi
doneRepository: OpenStrap/edge
Length of output: 2058
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sha='e047c5920ea2e0def28f028d0fc990c1fe64cd6b'
archive="https://codeload.github.com/OpenStrap/analytics/tar.gz/${sha}"
printf '%s\n' '--- workout-related files ---'
curl -fsSL "$archive" |
tar -tzf - |
rg 'src/onehz/workout|onehz/workout|strain'
printf '%s\n' '--- scorer declarations ---'
curl -fsSL "$archive" |
tar -xOzf - --wildcards '*/lib/src/onehz/workout/*' 2>/dev/null |
rg -n -C 12 'strainScoreMetric|double\??\s+strainScore|strainScore\s*\('Repository: OpenStrap/edge
Length of output: 981
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sha='e047c5920ea2e0def28f028d0fc990c1fe64cd6b'
archive="https://codeload.github.com/OpenStrap/analytics/tar.gz/${sha}"
python3 - "$archive" <<'PY'
import io
import re
import sys
import tarfile
import urllib.request
url = sys.argv[1]
data = urllib.request.urlopen(url).read()
pattern = re.compile(r'(strainScoreMetric|strainScore)', re.I)
with tarfile.open(fileobj=io.BytesIO(data), mode='r:gz') as archive:
for member in archive.getmembers():
if not member.isfile() or not member.name.endswith('.dart'):
continue
text = archive.extractfile(member).read().decode('utf-8', errors='replace')
if not pattern.search(text):
continue
print(f'--- {member.name} ---')
lines = text.splitlines()
for index, line in enumerate(lines):
if pattern.search(line):
start = max(0, index - 8)
end = min(len(lines), index + 9)
for number in range(start, end):
print(f'{number + 1}: {lines[number]}')
print()
PYRepository: OpenStrap/edge
Length of output: 2438
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sha='e047c5920ea2e0def28f028d0fc990c1fe64cd6b'
archive="https://codeload.github.com/OpenStrap/analytics/tar.gz/${sha}"
tmp_archive="$(mktemp)"
trap 'rm -f "$tmp_archive"' EXIT
curl -fsSL "$archive" -o "$tmp_archive"
python3 - "$tmp_archive" <<'PY'
import re
import sys
import tarfile
pattern = re.compile(r'(strainScoreMetric|strainScore)', re.I)
with tarfile.open(sys.argv[1], mode='r:gz') as archive:
for member in archive.getmembers():
if not member.isfile() or not member.name.endswith('.dart'):
continue
text = archive.extractfile(member).read().decode('utf-8', errors='replace')
if not pattern.search(text):
continue
print(f'--- {member.name} ---')
lines = text.splitlines()
for index, line in enumerate(lines):
if pattern.search(line):
start = max(0, index - 8)
end = min(len(lines), index + 9)
for number in range(start, end):
print(f'{number + 1}: {lines[number]}')
print()
PYRepository: OpenStrap/edge
Length of output: 50370
Use an API available in the pinned analytics revision.
At e047c5920ea2e0def28f028d0fc990c1fe64cd6b, neither ana.strainScore nor ana.strainScoreMetric accepts wakeMinutes or female. Use a revision that defines this overload, or call strainScoreMetric(trimp) and return its value only when present.
🧰 Tools
🪛 GitHub Actions: test / 0_test.txt
[error] 68-68: Flutter analyze: The named parameter 'wakeMinutes' isn't defined (undefined_named_parameter).
[error] 68-68: Flutter analyze: The named parameter 'female' isn't defined (undefined_named_parameter).
🪛 GitHub Actions: test / test
[error] 68-68: flutter analyze failed: named parameter 'wakeMinutes' is not defined (undefined_named_parameter).
[error] 68-68: flutter analyze failed: named parameter 'female' is not defined (undefined_named_parameter).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/compute/strain_backfill.dart` around lines 65 - 69, Update the strain
calculation near the existing ana.strainScore call to use an API supported by
the pinned analytics revision: either upgrade to a revision defining the
wakeMinutes and female overload, or call strainScoreMetric(trimp) and return its
value only when non-null. Preserve the existing null and wake validation
behavior.
Source: Pipeline failures
Comment on lines
+95
to
+105
| // The DATA EDGE is the newest day on disk, matching how the pruner measures | ||
| // retention (never the wall clock — a multi-day flash backfill received in | ||
| // one sync must not be treated as old). Days at or after the cutoff still | ||
| // have raw and are LEFT for a real re-derive: writing a patched row at | ||
| // kAlgoVersion here would satisfy the derive gate, which matches | ||
| // algo_version EXACTLY, and a partial patch would stand in for a full | ||
| // re-derivation of the day. | ||
| final days = <String>[ | ||
| for (final r in strainRows) ?(r['date'] as String?), | ||
| ]..sort(); | ||
| final cutoff = _shiftDays(days.last, -rawRetentionDays); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Derive retention from the decoded-data edge.
days.last is the latest non-null strain series row, not the latest decoded record. metricSeries('strain') excludes null values. If recent raw days have no strain, this cutoff moves backward and the backfill skips older raw-pruned days permanently when _markDone() runs.
Use LocalDb.lastDecodedRecTs() as the retention edge. If no decoded data exists, process all eligible stored days. Add a test where decoded data is newer than the newest strain row.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/compute/strain_backfill.dart` around lines 95 - 105, Update the retention
cutoff calculation in the strain backfill flow around _markDone() to use
LocalDb.lastDecodedRecTs() as the decoded-data edge instead of days.last from
strainRows. When no decoded record exists, leave the cutoff unset so all
eligible stored days are processed; preserve existing retention-day shifting
when a decoded timestamp is available, and add coverage for decoded data newer
than the newest strain row.
Comment on lines
+111
to
+175
| for (final day in days) { | ||
| if (day.compareTo(cutoff) >= 0) continue; | ||
|
|
||
| final row = await LocalDb.dayResult(day); | ||
| // Already carries a row at the current version — rescaled on a prior pass. | ||
| if (row != null && | ||
| ((row['algo_version'] as num?)?.toInt() ?? 0) >= kAlgoVersion) { | ||
| continue; | ||
| } | ||
|
|
||
| final next = rescaledStrain( | ||
| trimp: trimpBy[day], | ||
| wornMin: wornBy[day], | ||
| tstMin: tstBy[day], | ||
| female: female, | ||
| ); | ||
| if (next == null) { | ||
| skipped++; | ||
| continue; | ||
| } | ||
|
|
||
| if (row == null) { | ||
| // A series row with no bundle behind it: still worth fixing the trend. | ||
| await LocalDb.putMetricSeriesValue(day, 'strain', next); | ||
| seriesDays++; | ||
| continue; | ||
| } | ||
|
|
||
| final payload = _decode(row['payload_json']); | ||
| if (payload == null) { | ||
| skipped++; | ||
| continue; | ||
| } | ||
| final scalars = payload['scalars']; | ||
| if (scalars is! Map) { | ||
| skipped++; | ||
| continue; | ||
| } | ||
| scalars['strain'] = next; | ||
|
|
||
| // The intraday curve is cumulative strain, one point per wake minute, built | ||
| // from per-sample HR that no longer exists — it cannot be rescaled, and its | ||
| // last point IS the old headline. A curve ending at 12.79 under a headline | ||
| // of 9.03 contradicts itself, so it is DROPPED rather than left to disagree. | ||
| final series = payload['series']; | ||
| if (series is Map) series.remove('strain_curve'); | ||
|
|
||
| final partial = (row['partial'] as num?)?.toInt() == 1; | ||
| await LocalDb.putDayResult( | ||
| dayId: day, | ||
| algoVersion: kAlgoVersion, | ||
| payloadJson: jsonEncode(payload), | ||
| windowJson: (row['window_json'] as String?) ?? '{}', | ||
| finalized: (row['finalized'] as num?)?.toInt() == 1, | ||
| skipped: (row['skipped'] as num?)?.toInt() == 1, | ||
| partial: partial, | ||
| rhr: (row['rhr'] as num?)?.toDouble(), | ||
| rmssd: (row['rmssd'] as num?)?.toDouble(), | ||
| readiness: (row['readiness'] as num?)?.toDouble(), | ||
| // `putDayResult` skips the series write for a partial row, so only count | ||
| // the trend as rewritten when it actually was. | ||
| series: {'strain': next}, | ||
| ); | ||
| bundleDays++; | ||
| if (!partial) seriesDays++; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make the historical rewrite conditional and atomic.
Another derivation isolate can write a complete v63 result after Line 114 reads the v62 row and before Line 159 calls putDayResult(). ConflictAlgorithm.replace then replaces that complete v63 payload with this stale v62 payload, removes its newly derived strain_curve, and can discard other current-version detail.
Add one database transaction that verifies no current-version row exists and writes the bundle and series only when that condition still holds. Coordinate the compute_freshness claim in the same transaction. Add an interleaving regression test.
Based on learnings, “Recomputation must be idempotent: repeated derivation with additional data must not duplicate baseline entries, drift persisted scalars, or append where replacement is required.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/compute/strain_backfill.dart` around lines 111 - 175, Make the historical
rewrite in the backfill flow atomic by replacing the separate read/write
sequence around LocalDb.dayResult and LocalDb.putDayResult with one database
transaction that rechecks absence of a current-version row before writing the
bundle and series; skip the stale rewrite when another derivation has already
produced version kAlgoVersion. Update the compute_freshness claim within that
same transaction, preserving idempotent replacement semantics and preventing
current payload details or strain_curve data from being overwritten. Add an
interleaving regression test covering a concurrent current-version write.
Source: Learnings
Comment on lines
+209
to
+215
| /// Shift a 'YYYY-MM-DD' label by [days] calendar days. | ||
| String _shiftDays(String day, int days) { | ||
| final t = DateTime.parse(day).add(Duration(days: days)); | ||
| final mm = t.month.toString().padLeft(2, '0'); | ||
| final dd = t.day.toString().padLeft(2, '0'); | ||
| return '${t.year}-$mm-$dd'; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -a '^day_label\.dart$' lib
rg -n -C 3 'dayLabelOf|todayLabel|localDayStartSec|localDayEndSec' lib/data
rg -n -C 3 '_shiftDays|backfillStrainScale' lib testRepository: OpenStrap/edge
Length of output: 15772
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- strain_backfill.dart ---'
sed -n '1,235p' lib/compute/strain_backfill.dart
printf '%s\n' '--- day_label.dart ---'
cat -n lib/data/day_label.dart
printf '%s\n' '--- relevant tests and package API references ---'
rg -n -C 5 'shiftDays|dayLabelOf|localDay(Start|End)Sec|DST|spring|fall|strainScore|wakeMinutes|female' test lib pubspec.yamlRepository: OpenStrap/edge
Length of output: 50373
🌐 Web query:
Dart DateTime.parse date-only string local UTC semantics and DateTime.add Duration days daylight saving time documentation
💡 Result:
In Dart, the DateTime.parse() method and DateTime.add() behavior are governed by specific rules regarding time zones and the definition of a "day" [1][2][3]. DateTime.parse Semantics When using DateTime.parse() on a string that does not explicitly include a time zone offset (e.g., "2026-08-12"), Dart interprets the result in local time [4][5][6]. If the string includes an explicit UTC offset (e.g., "2026-08-12T14:00:00Z" or "2026-08-12T14:00:00-0500"), Dart parses it as a specific point in time and, if an offset other than UTC is provided, converts it to the equivalent UTC time [1][7]. To force a string to be parsed as UTC when it lacks an explicit offset, a common convention is to append 'Z' to the string (e.g., DateTime.parse(dateString + 'Z')), which informs the parser to treat it as UTC [4][5]. DateTime.add and Daylight Saving Time (DST) The DateTime.add() method adds a Duration to a DateTime instance [2]. A Duration of "days" is strictly defined as 24 hours (86,400 seconds) [8][9][3]. It does not represent a calendar day [8]. Because DateTime.add() operates on absolute time, it is affected by daylight saving time transitions when using local time [2][9]. If a transition (e.g., clocks moving forward or backward) occurs during the interval added, the resulting local time of day may shift, or the calendar date may not match expectations (e.g., adding 1 day might result in the same calendar day or skip one) [2][8][9]. Best Practices To perform calendar-based calculations without DST interference, it is recommended to work exclusively with UTC DateTime objects [8][9][10]. Because UTC does not observe daylight saving time, adding 86,400 seconds consistently results in a shift of exactly one calendar day [8].
Citations:
- 1: https://api.dart.dev/dart-core/DateTime/parse.html
- 2: https://api.dart.dev/dart-core/DateTime/add.html
- 3: https://api.dartlang.org/stable/latest/dart-core/Duration-class.html
- 4: Should be able to parse DateTime as UTC. dart-lang/sdk#37420
- 5: https://stackoverflow.com/questions/61668530/flutter-dart-datetime-parsing-utc-and-converting-to-local
- 6: https://api.dart.dev/dart-core/DateTime-class.html
- 7: https://api.flutter.dev/flutter/dart-core/DateTime/parse.html
- 8: DateTime.add(Duration days) produces wrong date dart-lang/sdk#37449
- 9: https://stackoverflow.com/questions/62822615/dart-inconsistency-in-adding-days-to-dates
- 10: DateTime addDays dart-lang/sdk#53729
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- strain_backfill.dart ---'
cat -n lib/compute/strain_backfill.dart | sed -n '1,225p'
printf '%s\n' '--- strain backfill tests ---'
cat -n test/strain_rescale_backfill_test.dart | sed -n '120,225p'
printf '%s\n' '--- strainScore references ---'
rg -n -C 8 'ana\.strainScore|strainScore|wakeMinutes|female' lib/compute lib test -g '*.dart'Repository: OpenStrap/edge
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
if ! command -v dart >/dev/null 2>&1; then
echo "dart runtime unavailable"
exit 0
fi
TZ=America/New_York dart - <<'DART'
String legacy(String day, int days) {
final t = DateTime.parse(day).add(Duration(days: days));
final mm = t.month.toString().padLeft(2, '0');
final dd = t.day.toString().padLeft(2, '0');
return '${t.year}-$mm-$dd';
}
String calendar(String day, int days) {
final p = day.split('-').map(int.parse).toList();
final t = DateTime(p[0], p[1], p[2] + days);
final mm = t.month.toString().padLeft(2, '0');
final dd = t.day.toString().padLeft(2, '0');
return '${t.year}-${mm}-${dd}';
}
for (final day in ['2026-03-09', '2026-11-02']) {
print('$day - 3 days: legacy=${legacy(day, -3)}, '
'calendar=${calendar(day, -3)}');
}
DARTRepository: OpenStrap/edge
Length of output: 177
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from datetime import date, datetime, timedelta, timezone
from zoneinfo import ZoneInfo
tz = ZoneInfo("America/New_York")
day = date(2026, 3, 9)
local_midnight = datetime.combine(day, datetime.min.time(), tzinfo=tz)
elapsed_result = (local_midnight.astimezone(timezone.utc) - timedelta(days=3)).astimezone(tz)
calendar_result = datetime.combine(day + timedelta(days=-3), datetime.min.time(), tzinfo=tz)
print("fixed-duration result:", elapsed_result.isoformat(), elapsed_result.date())
print("calendar-date result:", calendar_result.isoformat(), calendar_result.date())
PYRepository: OpenStrap/edge
Length of output: 2023
Use local calendar arithmetic in _shiftDays.
Duration(days: days) adds fixed 24-hour periods. Across a DST transition, this can produce the wrong local date label and cutoff. Construct the target with DateTime(year, month, day + days) and format it with dayLabelOf(). Add a spring-forward test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/compute/strain_backfill.dart` around lines 209 - 215, Update _shiftDays
to use local calendar arithmetic by constructing the target with DateTime(year,
month, day + days) instead of adding Duration(days: days), then return the
formatted result through dayLabelOf(). Add a test covering a spring-forward DST
transition and verifying the expected date label.
Source: Coding guidelines
Comment on lines
+362
to
+364
| // Today's strain target, in the shape CoachData reads. Absent until | ||
| // `strainTarget` has a recovery value, which the surfaces already handle. | ||
| 'coach': coachToday(cd), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Include coach in the empty Today response.
Line 220 returns before _crossDay() runs. If no daily bundle or wake features exist while strain_coach exists, getToday() omits coach and the Coach surfaces lose the target.
Load _crossDay() before the early return. Include 'coach': coachToday(cd) in that return shape. Add a regression test for this path.
As per coding guidelines, “When adding or changing a capability, cover every call path, including all raw decode paths and all relevant export/session triggers.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/data/local_repository_impl.dart` around lines 362 - 364, Update
getToday() to call _crossDay() before its empty-response early return, and
include coachToday(cd) under the 'coach' key in that return shape. Preserve the
existing coach field in the normal response and add a regression test covering
strain_coach without a daily bundle or wake features.
Source: Coding guidelines
Comment on lines
+3266
to
+3275
| final lo = (v['target_min'] as num?)?.toDouble(); | ||
| final hi = (v['target_max'] as num?)?.toDouble(); | ||
| if (lo == null || hi == null) return null; | ||
| return { | ||
| 'strain_target': { | ||
| 'value': (lo + hi) / 2, | ||
| 'low': lo, | ||
| 'high': hi, | ||
| 'rationale': (v['rationale'] ?? '').toString(), | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject inverted strain-target bounds.
coachToday() accepts target_min: 18 and target_max: 13. It then emits low: 18 and high: 13, which is an invalid CoachData range.
lib/data/local_repository_impl.dart#L3266-L3275: Returnnullwhenlo > hi.test/coach_strain_target_wiring_test.dart#L84-L87: Add a regression expectation for reversed numeric bounds.
As per coding guidelines, “Behavior changes, especially regressions involving readiness, abstention, idempotence, synchronization, migrations, and lifecycle safety, must include regression tests.”
Proposed validation
- if (lo == null || hi == null) return null;
+ if (lo == null || hi == null || lo > hi) return null;📍 Affects 2 files
lib/data/local_repository_impl.dart#L3266-L3275(this comment)test/coach_strain_target_wiring_test.dart#L84-L87
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/data/local_repository_impl.dart` around lines 3266 - 3275, Reject
inverted strain-target bounds in the strain-target mapping logic by returning
null when lo is greater than hi, while preserving valid ranges. Add a regression
expectation for reversed numeric bounds in
test/coach_strain_target_wiring_test.dart at lines 84-87.
Source: Coding guidelines
Comment on lines
+136
to
+205
| group('backfillStrainScale — the stored history', () { | ||
| test('rescales a raw-pruned historical day in series AND bundle', () async { | ||
| await seedDay('2026-07-09', | ||
| trimp: 177.80394321843846, strain: 12.790964777435558, | ||
| wornMin: 827, tstMin: 216); | ||
| // Data edge, well inside the retention window — must be left for the | ||
| // engine to re-derive from raw rather than patched here. | ||
| await seedDay('2026-07-20', | ||
| trimp: 200, strain: 13.0, wornMin: 900, tstMin: 400, | ||
| finalized: false); | ||
|
|
||
| final r = await backfillStrainScale(female: false); | ||
| expect(r.seriesDays, 1); | ||
| expect(r.bundleDays, 1); | ||
|
|
||
| // The trend series now carries the rescaled value. | ||
| expect(await seriesValue('strain', '2026-07-09'), closeTo(9.03, 0.05)); | ||
| // …and so does the bundle the day-detail screen reads. | ||
| final row = await LocalDb.dayResult('2026-07-09'); | ||
| expect((row!['algo_version'] as num).toInt(), kAlgoVersion); | ||
| final scalars = (jsonDecode(row['payload_json'] as String) | ||
| as Map)['scalars'] as Map; | ||
| expect((scalars['strain'] as num).toDouble(), closeTo(9.03, 0.05)); | ||
| // TRIMP is the input, not the output — it must survive untouched. | ||
| expect((scalars['trimp'] as num).toDouble(), | ||
| closeTo(177.80394321843846, 1e-9)); | ||
| }); | ||
|
|
||
| test('leaves days inside the raw-retention window for a real re-derive', | ||
| () async { | ||
| // Patching these would write a row AT kAlgoVersion, and the derive gate | ||
| // matches algo_version EXACTLY — the engine would then skip the day and | ||
| // a partial patch would stand in for a full re-derivation. | ||
| expect(await seriesValue('strain', '2026-07-20'), closeTo(13.0, 1e-9)); | ||
| final row = await LocalDb.dayResult('2026-07-20'); | ||
| expect((row!['algo_version'] as num).toInt(), 62); | ||
| }); | ||
|
|
||
| test('drops the stale intraday curve rather than contradicting the headline', | ||
| () async { | ||
| // `series.strain_curve` is cumulative strain, one point per wake minute, | ||
| // and its last point IS the old headline (12.79 for 2026-07-09). It was | ||
| // built from per-sample HR that no longer exists, so it cannot be | ||
| // rescaled — and a curve ending at 12.79 under a headline of 9.03 is | ||
| // worse than no curve. The UI already renders a missing curve honestly. | ||
| final row = await LocalDb.dayResult('2026-07-09'); | ||
| final payload = jsonDecode(row!['payload_json'] as String) as Map; | ||
| final series = payload['series'] as Map?; | ||
| expect(series?['strain_curve'], isNull); | ||
| // Everything else in the block survives. | ||
| expect(series?['hr_curve'], isNotNull); | ||
| }); | ||
|
|
||
| test('is idempotent — a second run rewrites nothing', () async { | ||
| final again = await backfillStrainScale(female: false); | ||
| expect(again.seriesDays, 0); | ||
| expect(again.bundleDays, 0); | ||
| expect(await seriesValue('strain', '2026-07-09'), closeTo(9.03, 0.05)); | ||
| }); | ||
|
|
||
| test('a day with no stored TRIMP is skipped, not zeroed', () async { | ||
| await LocalDb.putComputeFreshness(kStrainRescaleKey, '{}'); | ||
| await seedDay('2026-06-01', | ||
| trimp: null, strain: 11.5, wornMin: 800, tstMin: 200); | ||
|
|
||
| final r = await backfillStrainScale(female: false, force: true); | ||
| expect(r.skipped, greaterThanOrEqualTo(1)); | ||
| // Left exactly as it was — an un-rescalable day must not become 0. | ||
| expect(await seriesValue('strain', '2026-06-01'), closeTo(11.5, 1e-9)); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make each backfill test independent.
Several tests depend on rows created by earlier tests. For example, the test at Line 164 expects 2026-07-20 to exist, and the tests at Lines 174 and 189 expect the first test to have already run.
Reset and seed the database in setUp(), or combine the ordered assertions into one test. This makes filtered and reordered test execution reliable.
As per coding guidelines, “Behavior changes, especially regressions involving readiness, abstention, idempotence, synchronization, migrations, and lifecycle safety, must include regression tests.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/strain_rescale_backfill_test.dart` around lines 136 - 205, Make the
tests in the backfillStrainScale group independent by resetting and seeding all
required database rows in setUp(), including both historical dates and their
expected payload state. Ensure each test can run alone or in any order, while
preserving the existing assertions for rescaling, raw-retention skipping, curve
removal, idempotence, and missing-TRIMP handling.
Source: Coding guidelines
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
1 participant
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 moved
Rides the analytics change that redefines the 0–21 headline as the TRIMP earned above a quiet-waking baseline, rather than whole-waking-day TRIMP through a log base 1.5. An inactive full-wear day scored 12.8/21 on a real bundle, because ~16 h of simply being awake accrues ~180 TRIMP and that curve is steepest near zero. 21 sat at TRIMP ~4987 (~35 h at 80 % HRR), so a marathon read ~15.8 and the top of the scale was decorative.
Real days, recomputed by the new code:
Both call sites pass the wake window the TRIMP was accumulated over, since that is what sets the baseline:
perMin.lengthin the engine and the pure pipeline, the window's own length for a manually logged session. Passing the observed length rather than an assumed full day is what stops a partial-wear day being charged a whole day's overhead. Sex is threaded with it so the baseline is priced by the same Banister constant that scored the TRIMP.The intraday strain curve picks up Banister's 0.64/0.86 scale coefficient. It had been inlining
exp(b*hrr)and dropping the coefficient entirely, accumulating a TRIMP 1.5625× the day's own — tolerable while it was only a shape, not once the headline nets against a baseline priced withbanisterY.Backfill
Raw 1 Hz substrate is pruned
rawRetentionDays(3) behind the data edge, so history cannot be re-derived from raw — the engine keeps the old row and logs "no substrate (raw pruned)". It does not need raw: strain is a pure function of(TRIMP, wake minutes, sex), andmetric_seriesalready storestrimp,worn_minandtst_minfor every derived day.The reconstruction is exact, not approximate: on a real bundle
series.strain_curvehas 611 points againstworn_min − tst_min = 827 − 216 = 611. The curve carries one point per wake minute, so the window reconstructed here is the one the pipeline fed the scorer.strain_backfill.dartrewritesmetric_series(trends, sparklines,v_daily, the coach's SQL surface, baselines) and writes a correctedday_resultrow at the new algo version — a version bump writes a new row, so the immutable-per-version invariant holds. It runs once before the sweep, guarded bycompute_freshness, and never fatally.Three deliberate choices, each pinned by a test:
kAlgoVersion, and the derive gate matchesalgo_versionexactly — a partial patch would stand in for a full re-derivation.0is a number, not an absence.series.strain_curveis dropped on rescaled days. It was built from per-sample HR that no longer exists and its last point is the old headline, so a curve ending at 12.79 under a headline of 9.03 contradicts itself. I looked at inverting the old map to recover per-point TRIMP, but that depends on which code version wrote the curve (the missing 0.64 coefficient) and I could not make it reliably correct.Separate bug, found while tracing the target the user actually sees
There were two strain targets with different key names and only one producer.
crossDayPipelineemitsstrain_coach({target_min, target_max, band}) into the insights map, which the Insights card reads.CoachDatareadscoach.strain_target({value, low, high}) — and nothing wrote acoachkey anywhere:sub('coach')inpayloads.dartwas the only occurrence of it in the tree.So Today's plan chip, the Coach screen's target tile and the home-screen widget all rendered nothing, silently, while a test fixture "covered" the shape production never emitted.
coachToday()bridges the two,getToday()emits it, and the fixture's shape is now something the app actually produces.Tests
1901 tests pass,
dart analyze lib testclean. New coverage:strain_rescale_backfill_test.dart(exact recompute against both real bundles, abstention on missing inputs, the retention guard, idempotency, skip-don't-zero, curve removal) andcoach_strain_target_wiring_test.dart(the emitted shape actually reachesCoachData).The one thing not verified here is the migration against a real device DB — tests run on in-memory sqlite with seeded rows, so the first real run is worth watching for the
[derive] strain rescale: N day(s) rebuiltlog line.Summary by CodeRabbit
New Features
Improvements