Skip to content

Log and retime completed workouts - #189

Merged
abdulsaheel merged 3 commits into
mainfrom
feat/manual-workout-logging
Aug 4, 2026
Merged

Log and retime completed workouts#189
abdulsaheel merged 3 commits into
mainfrom
feat/manual-workout-logging

Conversation

@abdulsaheel

@abdulsaheel abdulsaheel commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

User description

Why

A user reported an hour of training logging as ~25 minutes. Nothing was broken — the auto-detector reports an effort's hard-effort core, not its wall-clock. A minute only counts at >= RHR + 0.45*(HRmax-RHR) (hrrFloorFraction), a dip over 90 s breaks the span (maxDipS), and the survivor must hold >= 12 min (minSustainedMin). That tuning is correct for a "did you work out?" prompt — the comments record it being calibrated against a sedentary week where a looser gate fired 30 false windows — but warm-up, cool-down and inter-set rest fall out of it.

There was no way to say otherwise. The only session writer was startWorkout, which stamps DateTime.now(); the only edit on a saved session was its type.

What

Two ways in. An Add pill in the Workouts header, and the session's time window on the detail hero — now rendered as a range (Today · 4:00 PM – 4:42 PM) that taps through to the same form. A start time alone can't show that a window was clipped.

Scoring reuses what was already there. getWorkout already enriches from the 1 Hz substrate over [start_ts, end_ts], so a corrected window gets its HR curve, avg/min/max, zone bands, drift and recovery curve for free. Only strain and calories lacked a producer, and they go through the published methods the day pipeline already uses.

No kAlgoVersion bump. Sessions don't feed day_result — derivation reads them only as savedSpans (auto-detect exclusion) and to backfill hrr_bpm — so no analytics output moved. Verified the pinned analytics SHA f0d1153 actually contains every symbol used (banisterTrimp, estimateBoutCalories, Sex, WorkoutUserProfile) with matching signatures, per the v43 drift lesson.

The strain convergence (second commit)

Three things put a number on the 0–21 dial and only one used the documented method:

method 60 min @ 150 bpm
daily strain Banister → min(21, ln(trimp+1)/ln(1.5)) 11.62
live session strain += %HRR * 0.01/sec 25.33
auto-detected nothing written blank

2.18x apart, and the live figure is off the top of its own scale — it passes 21 after ~50 min, after which the gauge clamps to full while the number keeps climbing. Pre-existing, but manual logging puts both in one list side by side, so it needed settling rather than documenting.

Live strain is no longer accrued: accrueHr folds 1 Hz samples into per-minute means (the unit Banister weights) and recomputes through the shared strainFromPerMinuteHr. The in-progress minute counts, so the gauge moves inside the first 60 s.

Anchors are honest now. The old path substituted 30 y / 70 kg / 60 bpm on an empty profile, turning an absent input into a confident-looking score. Each is a term in the formula, so a missing one abstains — LiveWorkoutState.strain is nullable, the arc sits empty, the readout is a dash. _liveRestingHr prefers the measured nightly rhr and never falls back to 60.

Note for anyone who has read the bot answer circulating on this: StrainScorer.strain() / WorkoutDetector (the 0–100 Edwards map) are not what the app uses — edge never calls them. The only analytics workout symbol edge imports is AutoWorkoutDetector.motionPoints.

Also fixed

GPS routes now follow their session's window. routePoints had no window filter, so a retimed session kept its original map, distance, moving time and splits — narrow a 90-minute run to the 60 you actually ran and the card claimed 60 minutes over 90 minutes of GPS. Clipped to the window (±5 s, since GPS is ms-stamped and the window is whole seconds); under two surviving fixes yields no map rather than an orphaned pin. Route rows survive, so widening back restores it.

Confirming an auto-detected suggestion now scores it. That path hand-built a row with no strain and no calories, so every accepted suggestion landed showing blanks.

The write seam re-validates and throws ManualWindowException — the form validates live, but its snapshot of saved spans can go stale mid-edit and it isn't the only possible caller.

A bug worth recording

"Log a past workout" first went in as a fourth child of the start bottom sheet, where it was invisible and untappable. showModalBottomSheet defaults to isScrollControlled: false, capping it at 9/16 of the screen; the nine type tiles already wrap to three rows. It overflowed by 11 px and the row fell off the bottom edge — in release there are no overflow stripes, so nothing announced it. Hence the header pill, a note on startWorkoutFlow, and a test that fails if the sheet is grown again (verified by reintroducing the bug and watching it fail).

Testing

flutter analyze clean; 1200 tests green (was 1156 on this base), run against the pinned sibling SHAs with pubspec_overrides.yaml removed so it matches CI. Both commits verified to compile and pass standalone.

  • manual_session_test.dart — validation, per-minute folding, zone bands, the honesty contract, row building
  • manual_workout_repo_test.dart — the write path against a real DB, including that widening a clipped window actually raises the numbers, and the route clipping
  • live_strain_convergence_test.dart — the live accumulator; one test records the retired formula's 25.33 and asserts the >2x gap
  • workouts_header_actions_test.dart — header fits at 375/390 pt in both palettes; the sheet stays under its cap

Not in scope

Live calories have the same shape of problem — an inline Keytel copy falling back to 30 y / 70 kg / male where the offline scorer abstains. Left deliberately; separate user-visible change.

Retiming is realistically a within-3-days feature (rawRetentionDays = 3). Older windows save with null strain/calories and the form says so plainly.

🤖 Generated with Claude Code


PR Type

Enhancement, Bug fix, Tests


Description

  • Add "Log a past workout" and "Edit times" flows so athletes can correct auto-detected session windows

  • Unify all strain scoring onto one Banister TRIMP → log-squash method, retiring the uncited %HRR × 0.01/s live accrual that exceeded 21 after ~50 min

  • Enforce honesty contract: absent HR, RHR, age, weight, or sex yields null strain/calories, never a fabricated figure

  • Add 4 new test files covering pure policy, repo wiring, live strain convergence, and header action smoke tests


Diagram Walkthrough

flowchart LR
  A["Workouts header\n_AddButton / _StartButton"]
  B["ManualWorkoutScreen\n(date + time pickers)"]
  C["manual_session.dart\n(pure policy)"]
  D["LocalRepositoryImpl\nlogManualWorkout / setWorkoutWindow"]
  E["LocalDb.putSession\n(INSERT-OR-REPLACE)"]
  F["strainFromPerMinuteHr\n(Banister TRIMP → log-squash)"]
  G["LiveWorkoutState.accrueHr\n(per-minute accumulator)"]
  H["_logDetectedSession\n(confirmed suggestion)"]
  I["WorkoutDetailContent\n_WindowLabel tap → retime"]

  A -- "Log past" --> B
  B -- "validateManualWindow\ncomputeManualSessionStats" --> C
  C -- "buildManualSessionRow" --> D
  D -- "putSession" --> E
  C -- "strainFromPerMinuteHr" --> F
  G -- "recomputes on each HR sample" --> F
  H -- "logDetectedWorkout via repo" --> D
  I -- "showManualWorkoutScreen(editing:d)" --> B
Loading

File Walkthrough

Relevant files
Enhancement
3 files
manual_session.dart
New pure policy module for manual/retimed workout windows
+404/-0 
workouts_screen.dart
Add Add pill, retime affordance, and scored detected-session logging
+166/-34
manual_workout_screen.dart
New full-screen form for logging and retiming completed workouts
+445/-0 
Bug fix
1 files
app_state.dart
Replace live strain accrual with Banister TRIMP; add nightly RHR
refresh
+98/-11 
Tests
2 files
manual_session_test.dart
Pure-policy unit tests for validation, scoring, and honesty contract
+601/-0 
manual_workout_repo_test.dart
Repo-level integration tests against real sqflite for log/retime flows
+422/-0 
Additional files
7 files
local_repository.dart +38/-0   
local_repository_impl.dart +197/-1 
live_session_screen.dart +25/-9   
workout_types.dart +31/-0   
core_screens_test.dart +63/-0   
live_strain_convergence_test.dart +110/-0 
workouts_header_actions_test.dart +152/-0 

The strap's auto-detector reports an effort's HARD-EFFORT CORE, not its
wall-clock: a minute only counts at >= RHR + 0.45*(HRmax-RHR), a dip over
90 s breaks the span, and the survivor must hold >= 12 min. That tuning is
right for a "did you work out?" prompt — it was calibrated against a
sedentary week where a looser gate fired 30 false windows — but it means
warm-up, cool-down and inter-set rest fall out, so an hour of mixed-intensity
training routinely surfaces as ~25 minutes.

There was no way to say otherwise. The only session writer was startWorkout,
which stamps DateTime.now(), and the only edit on a saved session was its
type. Reported by a user whose hour of training logged as 25 minutes.

Two ways in now: an "Add" pill in the Workouts header, and the session's time
window on the detail hero, which now reads as a range ("Today - 4:00 PM -
4:42 PM") and taps through to the same form. A start time alone cannot show
that a window was clipped.

Scoring reuses what was already there: getWorkout enriches from the 1 Hz
substrate over [start_ts, end_ts], so a corrected window gets its HR curve,
avg/min/max, zone bands, drift and recovery curve for free. Only strain and
calories needed a producer, and they go through the published methods the
day-level pipeline already uses — Banister TRIMP -> log squash, Keytel 2005.
No kAlgoVersion bump: sessions do not feed day_result (derivation reads them
only as savedSpans and to backfill hrr_bpm), so no analytics output moved.

Honesty contract holds. Every anchor is a term in its formula, so a missing
one abstains rather than defaulting — including calories, where
estimateBoutCalories' own usedDefaultAnchors flag is honoured instead of
banking a figure built on a 60 bpm guess. A window past the ~3-day
decoded_onehz retention saves with null strain/calories and the form says so
plainly rather than showing silent blanks.

Also here:

* GPS routes now follow their session's window. routePoints had no window
  filter, so a retimed session kept its original map, distance, moving time
  and splits — narrow a 90-minute run to the 60 you actually ran and the card
  claimed 60 minutes over 90 minutes of GPS. Clipped to the window (+/-5 s,
  since GPS is ms-stamped and the window is whole seconds); under two
  surviving fixes yields no map rather than an orphaned pin. The route rows
  survive, so widening back restores it.

* Confirming an auto-detected suggestion now scores it. That path hand-built
  a row with no strain and no calories, so every accepted suggestion landed
  in the log showing blanks — the numbers were computable from the substrate
  the whole time, nothing was asking for them.

* The write seam re-validates and throws ManualWindowException. The form
  validates live, but its snapshot of saved spans can go stale mid-edit and
  it is not the only possible caller.

* pickWorkoutType moved to workout_types.dart, next to the table it renders,
  so the new form can reach it without the two screen files importing each
  other.

"Log a past workout" first went in as a fourth child of the start bottom
sheet, where it was invisible and untappable: the sheet defaults to
isScrollControlled:false, capping it at 9/16 of the screen, and the nine type
tiles already wrap to three rows. It overflowed by 11 px and the row fell off
the bottom edge — in release there are no overflow stripes, so nothing
announced it. Hence the header pill, a note on startWorkoutFlow, and a test
that fails if the sheet is grown again (verified by reintroducing the bug).
Three things put a number on the 0-21 strain dial and only one of them was
using the documented method:

  daily strain        Banister TRIMP -> min(21, ln(trimp+1)/ln(1.5))
  a live session      strain += %HRR * 0.01, accrued per second
  an auto-detected    nothing written at all

Measured for the same hour at 150 bpm (age 30, RHR 55): the live accrual
reads 25.33 where the canonical method reads 11.62. That is 2.18x, and it is
off the top of its own scale — it passes 21 after about 50 minutes at that
intensity, after which the gauge clamps to full while the number beside it
keeps climbing. "Workout strain" and "daily strain" looked like the same
quantity and were not comparable.

This is pre-existing, but manual logging puts both numbers in one list where
they sit next to each other, so it had to be settled rather than documented.

Live strain is no longer accrued. accrueHr folds 1 Hz samples into per-minute
means — the unit Banister weights — and recomputes through the shared
strainFromPerMinuteHr. The in-progress minute counts, so the gauge moves
inside the first 60 s instead of sitting at zero. (The auto-detected path was
routed through the same function in the previous commit.)

Anchors are honest now too. The old path substituted 30 y / 70 kg / 60 bpm
when the profile was empty, which turned an absent input into a
confident-looking score. Every one of those is a term in the formula, so a
missing one abstains: LiveWorkoutState.strain is nullable, the arc sits empty
and the readout is a dash. _liveRestingHr prefers the MEASURED nightly rhr
over the user-typed one and never falls back to 60 — the display-only
_restingHr still does, which is fine for copy but not as a term inside a
formula.

That nullability carries to WorkoutFinishSnapshot, so an unscored session
shows a dash on the finish card instead of a confident 0.0, and cannot
accidentally trip the top-workout PR check.

No kAlgoVersion bump: session strain lives in `sessions`, not day_result.

Live CALORIES have the same shape of problem — an inline Keytel copy that
falls back to 30 y / 70 kg / male where the offline scorer abstains. Left
alone deliberately; it is a separate user-visible change.

One test records the retired formula's 25.33 and asserts the >2x gap, so the
regression is on the record rather than only in this message.
@coderabbitai

coderabbitai Bot commented Aug 4, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@abdulsaheel, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 31 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 5c09596b-51b9-4d9a-acd2-a567d06645e3

📥 Commits

Reviewing files that changed from the base of the PR and between a53234b and b893f22.

📒 Files selected for processing (10)
  • lib/compute/manual_session.dart
  • lib/data/local_repository.dart
  • lib/data/local_repository_impl.dart
  • lib/state/app_state.dart
  • lib/ui/workouts/manual_workout_screen.dart
  • lib/ui/workouts/workouts_screen.dart
  • test/core_screens_test.dart
  • test/manual_session_test.dart
  • test/manual_workout_repo_test.dart
  • test/workouts_header_actions_test.dart
📝 Walkthrough

Walkthrough

This change adds manual workout entry and retiming, shared session validation and HR scoring, repository persistence, route clipping, detector suggestion retirement, personalized live strain, and related UI and integration tests.

Changes

Workout session lifecycle

Layer / File(s) Summary
Session policy and scoring
lib/compute/manual_session.dart, test/manual_session_test.dart
Adds manual-window validation, HR aggregation, zone calculation, Banister strain, calorie scoring, stable IDs, editable session rows, and suggestion retirement.
Repository persistence and route handling
lib/data/local_repository.dart, lib/data/local_repository_impl.dart, test/manual_workout_repo_test.dart
Adds manual and detected workout logging, retiming, saved spans, write-time validation, suggestion dismissal, and route clipping.
Live strain personalization
lib/state/app_state.dart, lib/ui/activity/live_session_screen.dart, test/live_strain_convergence_test.dart
Uses profile and resting-HR anchors for per-minute strain scoring. Missing anchors remain nullable through finish and display flows.
Manual entry and workout detail UI
lib/ui/workouts/manual_workout_screen.dart, lib/ui/workouts/workout_types.dart, lib/ui/workouts/workouts_screen.dart, test/core_screens_test.dart, test/workouts_header_actions_test.dart
Adds manual logging, workout-type selection, detected-workout persistence fallback, completed-session retiming, time-window display, and layout coverage.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Athlete
  participant ManualWorkoutScreen
  participant LocalRepository
  participant ManualSessionPolicy
  participant LocalDatabase
  Athlete->>ManualWorkoutScreen: Enter workout window and type
  ManualWorkoutScreen->>LocalRepository: Save or retime workout
  LocalRepository->>ManualSessionPolicy: Validate window and compute statistics
  ManualSessionPolicy-->>LocalRepository: Session row and nullable metrics
  LocalRepository->>LocalDatabase: Persist session
  LocalRepository-->>ManualWorkoutScreen: Return save result
  ManualWorkoutScreen-->>Athlete: Show success or validation error
Loading

Possibly related PRs

Suggested labels: Review effort 4/5

Suggested reviewers: dannymcc

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the primary changes: adding completed workout logging and retiming support.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit b893f22)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Fabricated strain fallback

In _WorkoutFinishScreenState, the share-card construction at line 1391 uses s.strain ?? 0 as the final fallback. s.strain is now correctly nullable (the PR's own fix), but the ?? 0 here re-introduces a fabricated "0.0" for a session the profile could not anchor — exactly the honesty violation the PR set out to remove. The _strainGauge and the live sheet stat both correctly render "—" for a null strain, but the share card will print a confident "0.0 STRAIN" for the same session. The trigger is any user whose profile lacks age or sex logging a workout and then tapping Share.

title: 'HR recovery (60s)',
Missing mounted guard

In _logDetectedSession, after logDetectedWorkout succeeds, the code reads back the saved row with LocalDb.session('auto:$start') and conditionally calls exportWorkoutToHealth. The appState! non-null assertion on line 567 is inside a branch that already checked appState?.repo != null (i.e. api != null), but appState itself is a nullable parameter [AppState? appState]. If api is non-null then appState is necessarily non-null too, so the assertion cannot actually crash — but the logic is fragile: a future refactor that makes repo a non-nullable field on a nullable AppState would silently make the assertion reachable. More concretely, the mounted check that guards health export in the live-session path is absent here; exportWorkoutToHealth is called unconditionally after an await, with no mounted guard, from a free function that has no State to check. If the widget tree is torn down between the await api.logDetectedWorkout(...) and the health export call, the export fires against a potentially disposed AppState.

if (saved != null && appState!.healthSyncEnabled) {
  unawaited(appState.exportWorkoutToHealth(saved));
}
heightCm default imputation

In computeManualSessionStats, when building WorkoutUserProfile for the calorie estimator, profile.heightCm ?? 170.0 is used as a fallback. The surrounding guard already requires hrMax != null && age != null && weightKg != null && sex != null before entering the calorie branch, but heightCm is not in that guard. If a user's profile has no height recorded, 170.0 is silently substituted and a calorie figure is produced and persisted — violating the honesty contract the PR explicitly documents ("Real anchors only — usedDefaultAnchors stays false"). The test "no weight → no Keytel calories" covers the weight case but there is no corresponding test for absent height, and the code path is live.

profile: ana.WorkoutUserProfile(

@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Latest suggestions up to b893f22
Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Fabricated height anchor used in calorie computation

profile.heightCm ?? 170.0 fabricates a height anchor (170 cm) when heightCm is
absent. The Keytel formula uses height to estimate body surface area, so a
substituted default produces a calorie figure that is not grounded in the user's
real data — violating the honesty contract stated in the file header. The guard
should require heightCm to be non-null, just as it already requires weightKg, age,
and sex.

lib/compute/manual_session.dart [313-321]

-if (hrMax != null && age != null && weightKg != null && sex != null) {
+final heightCm = profile.heightCm;
+if (hrMax != null && age != null && weightKg != null && sex != null && heightCm != null) {
   // Real anchors only — `usedDefaultAnchors` stays false, so we are never
   // persisting a kcal figure built on a fabricated 220/60.
   final bout = ana.Calories.estimateBoutCalories(
     wornTs,
     [for (final v in worn) v.toDouble()],
     profile: ana.WorkoutUserProfile(
       weightKg: weightKg,
-      heightCm: profile.heightCm ?? 170.0,
+      heightCm: heightCm,
Suggestion importance[1-10]: 7

__

Why: This is a genuine violation of the honesty contract stated in the file header — profile.heightCm ?? 170.0 silently substitutes a fabricated anchor when heightCm is absent, producing a calorie figure not grounded in real data. The fix is straightforward and consistent with how other anchors (weightKg, age, sex) are already guarded.

Medium
Avoid fabricating zero strain on finish export path

At the point where WorkoutSummaryArgs (or equivalent) is constructed for the
share/export path, s.strain ?? 0 fabricates a zero strain for a session that was
never scored — exactly the honesty violation the rest of this PR carefully avoids. A
consumer of this value (e.g. a share card or HealthKit export) will display "0.0" as
a confident score. Pass null (or omit the field) so the downstream renderer can show
"—" the same way _strainGauge now does.

lib/ui/activity/live_session_screen.dart [1391]

-  strain: (d?['strain'] as num?)?.toDouble() ?? s.strain ?? 0,
+  strain: (d?['strain'] as num?)?.toDouble() ?? s.strain,
Suggestion importance[1-10]: 7

__

Why: The PR explicitly changed s.strain from double to double? to avoid fabricating scores, but then adds ?? 0 at line 1391 for the share/export path, contradicting the honesty contract. The suggestion correctly identifies this inconsistency and proposes removing the ?? 0 fallback.

Medium
Fallback write bypasses scoring on transient errors

The fallback unscored write below the catch (_) block hand-builds a row using
putSession directly, bypassing buildManualSessionRow and the
NotificationCenter/health-export path that logDetectedWorkout uses. This means any
transient error (e.g. a momentary DB lock) silently produces a row with no strain,
no calories, and no health export — exactly the blank-numbers bug this PR set out to
fix. The fallback should re-throw or surface the error rather than silently writing
a degraded row, so the athlete can retry through the normal path.

lib/ui/workouts/workouts_screen.dart [571-584]

 } on ManualWindowException {
   // A REFUSAL is not a failure to retry differently. The window collides
   // with a session already in the log, so those minutes are recorded
   // already; falling through would write a duplicate under the same
   // `auto:` id AND strip it back to the blank strain/calories this change
   // set out to remove — bypassing the very rule the write seam enforces.
   // The athlete acted on the card, so retire it and stop.
   await LocalDb.dismissWorkoutSuggestion(s['id'] as String);
   return;
 } catch (_) {
-  // Fall through to the unscored write — a suggestion the athlete has
-  // explicitly accepted must land in the log either way.
+  // A transient error must not silently produce a degraded row — rethrow
+  // so the suggestion card stays visible and the athlete can retry.
+  rethrow;
 }
Suggestion importance[1-10]: 4

__

Why: The suggestion raises a valid concern about the fallback path silently producing a degraded row on transient errors. However, the PR's comment explicitly states "a suggestion the athlete has explicitly accepted must land in the log either way" — meaning the fallback is intentional design. Re-throwing would leave the suggestion card visible and force the user to retry, which may be worse UX than a saved-but-unscored entry. The tradeoff is debatable, making this a moderate-impact suggestion.

Low
Boolean save latch never reset on unmount path

saving is set to true before the await but is only reset to false inside the on
ManualWindowException and catch (
) branches. If the try block succeeds and
res['unscored'] == true, _saving is cleared correctly. However, if !mounted is true
after the await (the widget was disposed mid-save), _saving is never reset — the
boolean latch is left set with no finally to clear it. This matches the sticky-latch
pattern called out in AGENTS.md §4.3. Wrap the body in a try/finally to guarantee
_saving is always cleared.

lib/ui/workouts/manual_workout_screen.dart [182-231]

 Future<void> _save() async {
   if (_error != null || _saving) return;
   final api = context.read<AppState>().repo;
   if (api == null) return;
   setState(() {
     _saving = true;
     _notice = null;
     _savedUnscored = false;
   });
   try {
-    ...
+    final editingId = widget.editing?['id'] as String?;
+    final res = editingId != null
+        ? await api.setWorkoutWindow(editingId,
+            startTs: _startSec, endTs: _endSec)
+        : await api.logManualWorkout(
+            startTs: _startSec, endTs: _endSec, type: _type);
+    if (!mounted) return;
+    if (res['unscored'] == true) {
+      setState(() {
+        _saving = false;
+        _savedUnscored = true;
+        _notice =
+            'Saved. There is no heart-rate data left for that window, so '
+            'strain and calories will stay blank — the band only keeps '
+            'about three days of second-by-second data on the phone.';
+      });
+      return;
+    }
+    Navigator.of(context).pop(true);
   } on ManualWindowException catch (e) {
     if (mounted) {
       setState(() {
         _saving = false;
         _notice = e.error.message;
       });
       _loadSpans();
     }
   } catch (_) {
     if (mounted) {
       setState(() {
         _saving = false;
         _notice = "Couldn't save that workout. Please try again.";
       });
     }
+  } finally {
+    if (mounted && _saving) setState(() => _saving = false);
   }
 }
Suggestion importance[1-10]: 3

__

Why: While the _saving latch technically leaks when the widget is disposed mid-save, in practice the widget is gone so the stuck boolean has no observable effect on the user. The finally guard is a defensive improvement but the impact is minimal since a disposed widget won't be re-shown in this state.

Low
Fix misindented back-fill call on resume path

The unawaited(_refreshNightlyRhr()) call in the resume path is indented at the wrong
level — it sits inside the if (existing == null) block (or equivalent surrounding
scope) rather than after the session is reconstructed and _workoutRawBase is set.
This means on a resume the back-fill fires before activeWorkout is assigned, so
_refreshNightlyRhr's final w = activeWorkout check finds null and the anchor is
never written to the resumed session. Move the call to after activeWorkout is set,
matching the start path.

lib/state/app_state.dart [3790-3793]

       workoutId: id,
       type: (row['type'] as String?) ?? 'other',
       age: (user?['age'] as num?)?.round(),
       profile: Profile.fromMap(user),
       restingHr: _liveRestingHr,
     );
     ...
     _workoutRawBase = _liveRaw;
-// A first night may have been derived since init. This read finishes
-// after the session below is constructed, so it back-fills the anchor on
-// `activeWorkout` when it lands rather than blocking the start.
-unawaited(_refreshNightlyRhr());
+    // A first night may have been derived since init. This read finishes
+    // after the session below is constructed, so it back-fills the anchor on
+    // `activeWorkout` when it lands rather than blocking the start.
+    unawaited(_refreshNightlyRhr());
Suggestion importance[1-10]: 3

__

Why: The suggestion claims unawaited(_refreshNightlyRhr()) fires before activeWorkout is assigned, but looking at the diff the call is placed after _workoutRawBase = _liveRaw and the session construction. The 'existing_code' and 'improved_code' differ only in indentation, which in Dart doesn't affect execution semantics. The indentation concern may be valid for readability but the functional claim appears incorrect.

Low
General
Use measured nightly RHR directly when back-filling session anchor

_liveRestingHr returns _nightlyRhr ?? user-supplied, but at the point it is assigned
to w.restingHr the intent is specifically to fill the gap with the freshly loaded
_nightlyRhr. If _nightlyRhr is non-null (just set above) but user?['resting_hr'] is
also non-null, _liveRestingHr returns _nightlyRhr — correct. However if _nightlyRhr
somehow ends up null after the vals.isEmpty guard (e.g. a concurrent call clears
it), _liveRestingHr falls back to the user value, which may already have been the
anchor the session started with, making the w.restingHr == null guard meaningless.
Use _nightlyRhr directly here to be explicit about what is being back-filled.

lib/state/app_state.dart [3531-3547]

 Future<void> _refreshNightlyRhr() async {
   try {
     final vals = await LocalDb.trailingSeriesValues('rhr', 7);
     if (vals.isEmpty) return;
     _nightlyRhr = vals.last;
     // Adopt it into a session that started before this read completed, but
     // only to FILL A GAP — overwriting an anchor a running session was
     // already scored against would move its number mid-workout.
     final w = activeWorkout;
     if (w != null && w.restingHr == null) {
-      w.restingHr = _liveRestingHr;
+      w.restingHr = _nightlyRhr;
       notifyListeners();
     }
   } catch (_) {
     /* best effort — falls back to the user-supplied RHR, or abstains */
   }
 }
Suggestion importance[1-10]: 4

__

Why: The suggestion to use _nightlyRhr directly instead of _liveRestingHr is logically sound since _nightlyRhr was just set above the guard, making the intent clearer. However, the practical difference is minimal since _nightlyRhr is non-null at that point (guarded by vals.isEmpty check), and the concurrent-clear scenario described is unlikely in a single-threaded Dart event loop.

Low

Previous suggestions

Suggestions up to commit a53234b
CategorySuggestion                                                                                                                                    Impact
Possible issue
Await RHR refresh before reading it for scoring

_refreshNightlyRhr() is called with unawaited inside the resume path, but
_liveRestingHr is read synchronously just above when constructing LiveWorkoutState
(the restingHr: _liveRestingHr line). The refresh fires after the state is already
built, so the resumed session is scored against the stale _nightlyRhr from init —
the same problem the comment says it is fixing. The refresh must be awaited before
LiveWorkoutState is constructed, or at minimum before restingHr is read.

lib/state/app_state.dart [3780-3783]

           _workoutRawBase = _liveRaw;
     // A first night may have been derived since init, so pick up a measured
     // RHR before this session is scored against it.
-    unawaited(_refreshNightlyRhr());
+    await _refreshNightlyRhr();
           // Never overwrite a live timer reference without cancelling it.
Suggestion importance[1-10]: 7

__

Why: The unawaited(_refreshNightlyRhr()) in the resume path fires after restingHr: _liveRestingHr is already read when constructing LiveWorkoutState, so the resumed session is scored against the stale _nightlyRhr. Awaiting the refresh before the state is built would fix this real ordering bug, though the practical impact depends on how often _nightlyRhr changes between init and resume.

Medium
Absent strain fabricates zero instead of abstaining

When d?['strain'] is absent and s.strain is null (profile lacked Banister anchors),
this falls back to 0, which fabricates a confident "0.0" strain score — exactly the
honesty violation the rest of this PR is fixing. The ShareWorkoutCard / export path
should receive null and render "—" rather than a zero that looks like a real score.

lib/ui/activity/live_session_screen.dart [1391]

-strain: (d?['strain'] as num?)?.toDouble() ?? s.strain ?? 0,
+strain: (d?['strain'] as num?)?.toDouble() ?? s.strain,
Suggestion importance[1-10]: 7

__

Why: The PR explicitly changes s.strain from non-nullable to nullable to avoid fabricating a "0.0" for unscored sessions, but then adds ?? 0 as a final fallback in the ShareWorkoutCard path, reintroducing the exact problem. The suggestion correctly points out this inconsistency and proposes removing the ?? 0 fallback.

Medium
Avoid per-sample Banister recompute on UI isolate

strainFromPerMinuteHr is called on every single 1 Hz HR sample inside accrueHr,
which runs on the UI isolate. For a one-hour session this is 3600 calls, each
recomputing Banister TRIMP over the full growing _perMinuteHr list — O(n²) total
work on the main thread. Strain only changes when a new minute completes; recompute
it only when _perMinuteHr actually grows (i.e. when the minute bucket rolls over),
not on every raw sample.

lib/state/app_state.dart [4152-4156]

-  strain = strainFromPerMinuteHr(
-      perMinuteHr(),
-      profile: profile,
-      restingHr: restingHr,
-    );
+    _minuteSum += hr;
+    _minuteCount++;
 
+    // Recompute strain only when a new completed minute is available —
+    // calling strainFromPerMinuteHr on every 1 Hz sample is O(n²) on the UI isolate.
+    // The in-progress partial minute is included via perMinuteHr() at read time.
+    if (minute != _minuteBucket) {
+      strain = strainFromPerMinuteHr(
+        perMinuteHr(),
+        profile: profile,
+        restingHr: restingHr,
+      );
+    }
+
Suggestion importance[1-10]: 6

__

Why: Calling strainFromPerMinuteHr on every 1 Hz sample causes O(n²) TRIMP computation on the UI isolate over a long session. However, the improved_code has a logic error — it checks if (minute != _minuteBucket) after _minuteBucket has already been updated to minute, so the condition would never be true at the rollover point. The fix concept is valid but the implementation in the suggestion is incorrect.

Low
General
Explicitly guard calories against absent resting HR

When restingHr is null, estimateBoutCalories receives a null restingHr and will
internally substitute a default anchor, causing usedDefaultAnchors to be true —
which correctly suppresses the result. However, the test "no resting HR → neither
strain nor calories" relies on this flag being set, making the honesty contract
dependent on the analytics library's internal behaviour rather than an explicit
guard in this codebase. The guard should be explicit: skip the calories call
entirely when restingHr is null, consistent with how strainFromPerMinuteHr handles
the same case.

lib/compute/manual_session.dart [300-316]

-    if (hrMax != null && age != null && weightKg != null && sex != null) {
+    if (hrMax != null && age != null && weightKg != null && sex != null && restingHr != null) {
       final bout = ana.Calories.estimateBoutCalories(
         hrTs,
         [for (final v in hrBpm) v.toDouble()],
         profile: ana.WorkoutUserProfile(
           weightKg: weightKg,
           heightCm: profile.heightCm ?? 170.0,
-          ...
+          age: age,
+          sex: sex == 'f' || sex == 'female' ? 'female' : 'male',
         ),
         hrmax: hrMax,
         restingHr: restingHr,
       );
       if (!bout.usedDefaultAnchors && bout.kcal > 0) calories = bout.kcal;
     }
Suggestion importance[1-10]: 5

__

Why: Adding restingHr != null to the guard makes the honesty contract explicit in this codebase rather than relying on the analytics library's usedDefaultAnchors flag. The current code already correctly suppresses the result via that flag, so this is a defensive improvement to code clarity and robustness rather than a bug fix.

Low
Trailing RHR window returns oldest instead of newest

trailingSeriesValues returns the trailing N rows ordered newest-first (or
newest-last, depending on the implementation), but vals.last may be the oldest value
in the window rather than the most recent. Per AGENTS.md, trailingSeriesValues(key,
n) is the correct helper for a trailing window, but the caller should use vals.first
to get the most recent nightly RHR, not vals.last, which would anchor TRIMP to a
week-old figure.

lib/data/local_repository_impl.dart [2186-2188]

 final vals = await LocalDb.trailingSeriesValues('rhr', 7);
 if (vals.isEmpty) return null;
-return vals.last;
+return vals.first;
Suggestion importance[1-10]: 5

__

Why: This is a potentially valid concern — using vals.last vs vals.first depends on the ordering of trailingSeriesValues, which isn't visible in the diff. If the values are ordered newest-first, vals.last would indeed return the oldest value, anchoring TRIMP to a stale figure. However, the suggestion assumes an ordering that isn't confirmed in the PR code.

Low
Absent strain renders misleading zero-filled arc

When strain is null the arc is drawn at 0.0, which renders a full empty ring —
visually indistinguishable from a scored session with zero strain. The gauge should
communicate "no data" differently; passing null (or a sentinel like -1) to ArcGauge
and letting it render a dashed/greyed arc, or simply hiding the gauge entirely,
avoids the fabricated-zero appearance. At minimum, the readout already shows "—" so
the arc should not imply a zero value.

lib/ui/activity/live_session_screen.dart [1042]

-value: strain == null ? 0.0 : (strain / 21).clamp(0.0, 1.0),
+value: strain == null ? null : (strain / 21).clamp(0.0, 1.0),
Suggestion importance[1-10]: 4

__

Why: While the concern about visual ambiguity is valid, the improved_code passes null to ArcGauge.value, which likely expects a double — this would require changes to ArcGauge itself that are outside the PR scope. The suggestion is speculative without knowing ArcGauge's API, making the improved_code potentially incorrect.

Low

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 12

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
lib/state/app_state.dart (1)

3575-3595: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Capture the refreshed RHR anchor before creating LiveWorkoutState.

_refreshNightlyRhr() assigns _nightlyRhr only after its database await. Both flows construct LiveWorkoutState with the pre-refresh _liveRestingHr, and restingHr is final. A nightly RHR derived after initialization cannot score the current session when no manual RHR exists, and stopWorkout() then persists null strain.

Load the anchor before constructing either state. If startup becomes asynchronous, re-check activeWorkout after the await and update all start callers. Add a delayed-loader regression test for both new and resumed sessions.

As per coding guidelines, cover every call path when adding or changing a capability.

Also applies to: 3770-3782

🤖 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/state/app_state.dart` around lines 3575 - 3595, Ensure both new and
resumed session start flows await _refreshNightlyRhr() before constructing
LiveWorkoutState, so restingHr uses the refreshed anchor when no manual RHR
exists. Because startup becomes asynchronous, re-check activeWorkout after the
await and update every start caller accordingly; add delayed-loader regression
coverage for both paths.

Source: Coding guidelines

🤖 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/manual_session.dart`:
- Around line 300-316: Apply the worn-sample filter to the calorie calculation
in computeManualSessionStats by building timestamped worn pairs and passing
their timestamps and heart rates to ana.Calories.estimateBoutCalories instead of
the raw hrTs and hrBpm; update test/manual_session_test.dart lines 344-357 to
assert the dropped-minute window produces the same calories as the equivalent
worn-only window.

In `@lib/data/local_repository_impl.dart`:
- Around line 2038-2043: Update the type guard that constructs SessionSpan so
both start_ts and end_ts are checked as num, matching the existing numeric casts
and allowing double values to be retained. Keep the id String check and
SessionSpan conversion unchanged.
- Around line 2083-2097: Update setWorkoutWindow to reject existing sessions
whose status is live before calling _writeManualSession, preserving the existing
behavior for non-live sessions. Use the existing row’s status value and throw an
appropriate StateError at this repository write seam.

In `@lib/data/local_repository.dart`:
- Around line 118-148: Update the return-value documentation for
logManualWorkout, logDetectedWorkout, and setWorkoutWindow to include the
hr_samples key alongside workout_id and unscored, matching
LocalRepositoryImpl._writeManualSession and the existing test contract.

In `@lib/ui/workouts/manual_workout_screen.dart`:
- Around line 116-127: Update _pickDate so showDatePicker always receives an
initialDate within its firstDate/lastDate range: clamp _start to firstDate when
it predates the allowed boundary, while preserving the existing date limits and
picker behavior.
- Around line 339-351: Update _dateLabel to use dayLabelOf and todayLabel from
data/day_label.dart for the Today/Yesterday decision instead of comparing
DateTime differences with inDays. Compare local calendar-day labels, preserve
the existing Today/Yesterday and month-day display behavior, and follow the
repository’s day-label helpers without assuming fixed day lengths.

In `@lib/ui/workouts/workouts_screen.dart`:
- Around line 561-580: Update the workout logging try/catch around
logDetectedWorkout so ManualWindowException from _writeManualSession is rethrown
and never reaches the hand-built fallback row. Keep fallback behavior for other
unexpected errors, and add the compute/manual_session.dart import if needed;
preserve the successful path that dismisses the suggestion and exports the saved
session.

In `@test/core_screens_test.dart`:
- Around line 468-473: Update the positive retime-affordance test around the
tapped finder to use find.bySemanticsLabel(RegExp('Edit workout times')) instead
of find.textContaining('–'). Keep the existing tap and assertion unchanged so
both positive and negative cases target the same semantics-labeled control
without relying on tree order.

In `@test/manual_session_test.dart`:
- Around line 344-357: Add a calories assertion to the off-skin zero-sample test
for computeManualSessionStats, verifying that zero heart-rate samples do not
alter the expected calorie result alongside avgHr and hrSampleCount.

In `@test/manual_workout_repo_test.dart`:
- Around line 335-355: The tests “retiming preserves the route rows themselves
(id is stable)” and “savedSessionSpans surfaces saved windows for the overlap
check” depend on data created by earlier tests. Seed the required w-route rows
inside the first test and create a saved session/window inside the second before
calling LocalDb.routePoints or repo.savedSessionSpans, so each test passes
independently without relying on shared test state.
- Around line 364-374: Update the rejected-write assertions in the overlap,
future-window, and sub-minute tests to await the asynchronous expectation using
await expectLater before querying the database. Preserve the existing
ManualWindowException and error assertions, and ensure each “Nothing was
written” check runs only after logManualWorkout has completed and been rejected.

In `@test/workouts_header_actions_test.dart`:
- Around line 129-132: Add a unique key to the workout sheet body’s SafeArea and
update the height assertion in the relevant test to locate that widget by key
instead of using find.byType(SafeArea).last. Keep the existing size threshold
and assertion behavior unchanged.

---

Outside diff comments:
In `@lib/state/app_state.dart`:
- Around line 3575-3595: Ensure both new and resumed session start flows await
_refreshNightlyRhr() before constructing LiveWorkoutState, so restingHr uses the
refreshed anchor when no manual RHR exists. Because startup becomes
asynchronous, re-check activeWorkout after the await and update every start
caller accordingly; add delayed-loader regression coverage for both paths.
🪄 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: b367c34a-5f94-4115-816e-78a5f61f66a0

📥 Commits

Reviewing files that changed from the base of the PR and between b2a9812 and a53234b.

📒 Files selected for processing (13)
  • lib/compute/manual_session.dart
  • lib/data/local_repository.dart
  • lib/data/local_repository_impl.dart
  • lib/state/app_state.dart
  • lib/ui/activity/live_session_screen.dart
  • lib/ui/workouts/manual_workout_screen.dart
  • lib/ui/workouts/workout_types.dart
  • lib/ui/workouts/workouts_screen.dart
  • test/core_screens_test.dart
  • test/live_strain_convergence_test.dart
  • test/manual_session_test.dart
  • test/manual_workout_repo_test.dart
  • test/workouts_header_actions_test.dart

Comment thread lib/compute/manual_session.dart
Comment thread lib/data/local_repository_impl.dart Outdated
Comment thread lib/data/local_repository_impl.dart
Comment thread lib/data/local_repository.dart
Comment thread lib/ui/workouts/manual_workout_screen.dart
Comment thread test/core_screens_test.dart Outdated
Comment on lines +468 to +473
final window = find.textContaining('–');
expect(window, findsWidgets);

await t.tap(window.first);
await t.pump();
expect(tapped, 1);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Target the retime affordance by its semantics label.

find.textContaining('–') also matches the zone rows, which render '${lo}–${hi} bpm' with the same en dash. The test then relies on tree order for window.first. The negative test below already uses find.bySemanticsLabel(RegExp('Edit workout times')); use the same finder here so the positive and negative cases assert the same thing.

♻️ Proposed change
-      // A range, not just the start — "Today · 4:00 PM – 4:42 PM".
-      final window = find.textContaining('–');
-      expect(window, findsWidgets);
-
-      await t.tap(window.first);
+      // A range, not just the start — "Today · 4:00 PM – 4:42 PM".
+      final window = find.bySemanticsLabel(RegExp('Edit workout times'));
+      expect(window, findsOneWidget);
+
+      await t.tap(window);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
final window = find.textContaining('–');
expect(window, findsWidgets);
await t.tap(window.first);
await t.pump();
expect(tapped, 1);
final window = find.bySemanticsLabel(RegExp('Edit workout times'));
expect(window, findsOneWidget);
await t.tap(window);
await t.pump();
expect(tapped, 1);
🤖 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/core_screens_test.dart` around lines 468 - 473, Update the positive
retime-affordance test around the tapped finder to use
find.bySemanticsLabel(RegExp('Edit workout times')) instead of
find.textContaining('–'). Keep the existing tap and assertion unchanged so both
positive and negative cases target the same semantics-labeled control without
relying on tree order.

Comment thread test/manual_session_test.dart
Comment thread test/manual_workout_repo_test.dart
Comment thread test/manual_workout_repo_test.dart Outdated
Comment thread test/workouts_header_actions_test.dart Outdated
Twelve inline findings plus one outside the diff. Verified each against the
code first; all were real, several in ways worth recording.

Correctness

* computeManualSessionStats dropped off-skin samples for avgHr, maxHr,
  hrSampleCount, strain and the zone bands, but forwarded the raw list to the
  calorie estimator — which bills every sample under the active threshold at
  the RESTING rate, so a window with lost contact bought basal kcal that every
  other field had correctly discarded. The filter now runs once and everything
  downstream reads the survivors. Production could not reach this
  (hrSamplesInRange filters hr > 0 in SQL) but nothing in the function said so.

* _logDetectedSession caught ManualWindowException and fell through to the
  hand-built row: a refusal became a duplicate write under the same auto: id,
  stripped back to the blank strain and calories this change set out to
  remove, bypassing the overlap rule the write seam exists to enforce. A
  refusal now retires the suggestion and stops; the fallback stays for genuine
  failures.

* showDatePicker asserts initialDate is on or after firstDate
  (date_picker.dart:235). firstDate was pinned to 1 January of last year while
  initialDate came from the session, so retiming anything older threw instead
  of opening the picker. Sessions are never pruned and an import can bring in
  years of history. The floor now widens to admit whatever is being shown.

* _dateLabel decided Today/Yesterday from today.difference(that).inDays. That
  is elapsed time between two local midnights, and a spring-forward day is
  23 h, which truncates to 0 — yesterday would render as "Today". Now compares
  dayLabelOf/todayLabel, per the repository's own day-label rule. NOTE: the
  pre-existing _dayLabel and _whenLabel in workouts_screen.dart carry the same
  pattern and are left alone here.

* startWorkout fired _refreshNightlyRhr() unawaited and then immediately
  constructed LiveWorkoutState with the pre-refresh anchor, so the comment
  claiming it picked up a measured RHR "before this session is scored" was
  simply false. restingHr is no longer final and the refresh back-fills it when
  the read lands — but only to fill a gap, since overwriting an anchor a
  running session was already scored against would move its number mid-workout.

* setWorkoutWindow accepted a status='live' row and buildManualSessionRow
  always writes 'done', so retiming a running session silently ended it. The
  detail screen already guarded this, but the window is re-validated at this
  seam precisely because the form is not the only caller.

* savedSessionSpans guarded on `is int` while casting `as num`; a double would
  have been dropped, silently disabling the overlap check for that session.

Tests

* Two tests read rows an earlier test happened to leave behind. Confirmed by
  running one alone: 0 route rows instead of >2. Both now seed their own data.

* Three rejection tests used expect() on an async callback, which returns
  before the future settles, so the "nothing was written" query raced the
  rejection. Now await expectLater.

* The off-skin test asserted avgHr and hrSampleCount but not calories — the
  exact field that was wrong. It now pins calories, strain and zones against an
  equivalent worn-only window.

* The retime-affordance test matched find.textContaining('–'), which also
  matches the zone rows ("133–152 bpm"), so it relied on tree order. Both cases
  now target a Key.

* The sheet-height assertion used find.byType(SafeArea).last, which depended on
  how many SafeAreas the harness nested.

Not done, deliberately

The review suggested targeting the affordance by semantics label. That finder
returns nothing here: the annotation merges into an ancestor card's node, and
tester.getSemantics resolves to that ancestor (a 350x275 rect) rather than to
the label. container: true and excludeSemantics: true were tried to force a
standalone node and could not be shown to work. Excluding the child's semantics
on that evidence would risk dropping the window text from the merged
announcement while gaining nothing — the same trap as excluding semantics
without re-exposing the action. Left as a plain annotation with a note, and the
tests target a Key instead. This also means the ORIGINAL negative assertion
(bySemanticsLabel, findsNothing) was passing for the wrong reason, since that
finder could never have matched.

flutter analyze clean; 1201 tests green against the pinned sibling SHAs.
@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit b893f22

@abdulsaheel
abdulsaheel merged commit d911f60 into main Aug 4, 2026
3 checks passed
@abdulsaheel
abdulsaheel deleted the feat/manual-workout-logging branch August 4, 2026 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant