Skip to content

Fix #170: cycling/rowing workouts weren't being suggested - #171

Merged
abdulsaheel merged 2 commits into
mainfrom
fix/issue-170-cycling-workout-detection
Jul 31, 2026
Merged

Fix #170: cycling/rowing workouts weren't being suggested#171
abdulsaheel merged 2 commits into
mainfrom
fix/issue-170-cycling-workout-detection

Conversation

@abdulsaheel

@abdulsaheel abdulsaheel commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

User description

Fixes #170.

Root cause

autoDetectWorkouts's motion-confirmation gate (motionConfirmMean = 0.15, mean L2 gravity-delta/s) is tuned for arm-swing activities like walking/running. It unconditionally drops any sustained-elevated-HR window where the wrist stays still — exactly what happens gripping a handlebar/oar during cycling/rowing — regardless of how strong the HR signal is. That's why a real 1h50m road cycling session (avg ~117W, HR 100-140, peak 157 bpm) produced no suggestion at all.

Fix

  • Depends on autoDetectWorkouts: bypass motion gate on genuine HR onset (edge#170) analytics#32 (pinned above at its PR-branch head — repin to main once that merges): AutoWorkoutDetector now bypasses the motion gate on a genuine HR onset — mean bpm over the first 3 min of the candidate span must rise ≥25 bpm vs. the 3 min immediately before it (Whipp & Wasserman 1972 phase-II kinetics: real exertion shows a fast HR rise right at onset).
  • A raw %HRR-level bypass was tried and rejected first: it reopens the exact false positive the motion gate exists to catch, since level alone can't distinguish "just started exercising" from "already elevated" (fever, anxiety, heat). The onset check does distinguish them — a slow drift shows no sharp rise and stays gated.
  • kAlgoVersion 50 → 51 (this changes which suggestions autoDetectWorkouts emits).

Test plan

  • dart test in analytics — 386 passed, no failures (see analytics#32 for the new onset-gate tests).
  • flutter test --concurrency=1 — 1055 passed, no regressions.
  • flutter analyze — clean.
  • Verified the new pin resolves (pubspec.lock resolved-ref matches) with pubspec_overrides.yaml moved aside, per the repo's CI-reproduction convention.

PR Type

Bug fix


Description


Diagram Walkthrough

flowchart LR
  A["Motion gate\n(arm-swing tuned)"]
  B["Cycling/rowing window\n(wrist still on handlebar/oar)"]
  C["HR-onset check\n(≥25 bpm rise in first 3 min)"]
  D["Workout suggestion\nemitted"]
  E["Slow-drift elevation\n(fever/heat/anxiety)"]
  F["Suggestion suppressed\n(no onset detected)"]
  B -- "previously dropped" --> A
  B -- "now: onset check" --> C
  C -- "fast rise detected" --> D
  E -- "no sharp rise" --> F
Loading

File Walkthrough

Relevant files
Bug fix
derivation_engine.dart
Bump kAlgoVersion to 51 for workout detection fix               

lib/compute/derivation_engine.dart

  • Bumps kAlgoVersion from 50 to 51
  • Adds detailed changelog comment explaining the HR-onset bypass fix for
    cycling/rowing detection
  • Documents that the analytics#32 pin is a PR-branch head pending merge
    to main
+13/-1   
Dependencies
pubspec.yaml
Repin analytics to HR-onset bypass PR-branch head               

pubspec.yaml

  • Repins openstrap_analytics git ref from 5d421918... (analytics#31 tip)
    to 422c8b8b... (analytics#32 PR-branch head)
  • Updates comment to note this is a PR-branch head, not main, and must
    be repinned to merge commit once analytics#32 lands
  • Documents verification method for the new pin (onsetRiseBpm presence
    in auto_detect.dart)
+10/-7   

Summary by CodeRabbit

  • Bug Fixes
    • Improved workout auto-detection for activities with limited lower-body movement.
    • Added a heart-rate rise threshold to distinguish genuine exercise from gradual, non-exercise elevations.
  • Improvements
    • Updated analytics behavior to improve workout detection accuracy and verification.

autoDetectWorkouts' motion-confirmation gate was tuned for arm-swing
activities and silently dropped every low-limb-swing cardio window
(cycling/rowing — wrist stays still on a handlebar/oar) regardless of
HR signal strength. Repins to analytics#32 (fix/issue-170-...), which
adds an HR-onset bypass instead of loosening the raw %HRR bar (that
was tried and rejected upstream — it reopens false positives from
fever/anxiety/heat plateaus).

kAlgoVersion 50 -> 51 (analytics-output change: which suggestions
autoDetectWorkouts emits).

PR branch head, not main — analytics#32 is still open. Repin to the
merge commit once it lands.
@coderabbitai

coderabbitai Bot commented Jul 31, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 61d8deca-ee4b-410a-a74d-c21f72ae1973

📥 Commits

Reviewing files that changed from the base of the PR and between 4976e60 and 52b8e9b.

⛔ Files ignored due to path filters (1)
  • pubspec.lock is excluded by !**/*.lock
📒 Files selected for processing (1)
  • pubspec.yaml

📝 Walkthrough

Walkthrough

The PR increments the derivation algorithm version to 51 and pins openstrap_analytics to a PR branch that implements the HR-onset workout auto-detection behavior.

Changes

Workout detection analytics

Layer / File(s) Summary
Align algorithm version and analytics revision
lib/compute/derivation_engine.dart, pubspec.yaml
The algorithm version changes from 50 to 51. The analytics dependency changes to the PR branch that documents the three-minute, 25 BPM heart-rate rise condition for bypassing motion confirmation.

Estimated code review effort: 2 (Simple) | ~10 minutes

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 identifies the fix for missing cycling and rowing workout suggestions, which matches the pull request objective.
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 Jul 31, 2026

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit 52b8e9b)

Here are some key observations to aid the review process:

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

PR-branch pin

The analytics dependency is pinned to a PR-branch head (cbbe06a) rather than a merged main commit. AGENTS.md §3.5/§4.9 explicitly forbids this: "A bump citing a sibling change must be backed by the pin" and siblings must be "pinned to full commit SHAs, never branch refs." The comment itself acknowledges this ("PR-BRANCH HEAD, not main — repin once it lands"). If analytics#32 is rebased, force-pushed, or the branch is deleted after merge, flutter pub get will fail or silently resolve a different tree. More critically, kAlgoVersion 51 is now live in production builds pointing at an unmerged, mutable ref — if the analytics PR is amended before merge, the version bump and the actual behavior will diverge, exactly the scenario that kept v43's bug live for three releases. This should not ship until the analytics PR is merged and the pin is updated to the merge commit on main.

# PR-BRANCH HEAD, not main — OpenStrap/analytics#32 (fix/issue-170-...)
# is open, not yet merged. Repin to the merge commit on main once it
# lands (same pattern v49 used briefly for protocol/analytics PR heads).
# autoDetectWorkouts now bypasses the motion-confirmation gate on a
# genuine HR-onset (fast rise vs. immediately-preceding baseline),
# fixing low-limb-swing cardio (cycling/rowing) going undetected while
# still rejecting slow-drifting elevations (fever/heat/anxiety) that
# have no discernible onset.
# Verified present at THIS sha: `git show <sha>:lib/src/onehz/workout/auto_detect.dart | grep onsetRiseBpm`.
# Moved to the branch's new head (cbbe06a) after CodeRabbit caught a real
# off-by-one in the onset window on analytics#32 (earlyMean was 181s vs
# preMean's 180s) — fixed there, not worth a separate edge changelog line.
ref: cbbe06addec1cb78b4ea2c75f64e8a281ac09294
Version bump without test

kAlgoVersion is bumped from 50 to 51, which changes which workout suggestions autoDetectWorkouts emits. AGENTS.md §5 notes that behavior changes with no accompanying test in test/ are a real finding because no CI gate runs on PRs. The analytics-side tests live in the analytics repo and are not part of this repo's flutter test suite. There is no new test in test/ pinning the edge-side behavior change (e.g., that a cycling-shaped HR trace now produces a suggestion, or that a fever-shaped slow-drift trace still does not). Following the repo convention of naming tests for the bug they pin, a cycling_workout_detection_test.dart is missing.

// v51: edge#170 — autoDetectWorkouts' motion-confirmation gate (tuned for
// arm-swing activities) silently dropped every low-limb-swing cardio window
// (cycling/rowing: the wrist stays still on a handlebar/oar) no matter how
// strong the HR signal was. analytics#32 (PR-branch head, pinned above —
// repin to main once merged) adds an HR-ONSET bypass: the gate is skipped
// only when mean bpm over the candidate's first 3 min rises >=25 bpm versus
// the 3 min immediately before it (Whipp & Wasserman 1972 phase-II kinetics),
// which fires on genuine exercise starts but NOT on slow-drifting elevations
// (fever/heat/anxiety) that have no discernible onset — so this changes which
// suggestions autoDetectWorkouts emits without loosening the false-positive
// gate it exists to protect.
const int kAlgoVersion = 51;

@github-actions

github-actions Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Latest suggestions up to 52b8e9b
Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Pin sibling to merged main SHA, not branch head

The comment explicitly states this is a PR-branch head ("not yet merged"), which
violates the hard invariant §3.6 that siblings must be pinned to full commit SHAs on
main, never branch refs or unmerged PR heads. Shipping an unmerged PR-branch SHA
means the pin can be silently invalidated if the branch is force-pushed, and the
changelog entry in kAlgoVersion v51 describes a fix that is not yet in main. This is
the exact pattern that caused v43's bug to stay live for three releases. The PR
should not be merged until analytics#32 lands on main and the pin is updated to that
merge commit SHA.

pubspec.yaml [64]

-ref: cbbe06addec1cb78b4ea2c75f64e8a281ac09294
+# Repin to the merge commit SHA on analytics main once analytics#32 lands.
+# Do NOT merge this edge PR until that SHA is available and verified:
+# `git show <merge-sha>:lib/src/onehz/workout/auto_detect.dart | grep onsetRiseBpm`
+ref: <analytics#32-merge-commit-sha-on-main>
Suggestion importance[1-10]: 3

__

Why: The comment in the PR explicitly acknowledges this is a temporary PR-branch pin and instructs to repin once merged — this is intentional and documented. The suggestion's improved_code replaces the real SHA with a placeholder <analytics#32-merge-commit-sha-on-main>, which would break the build and is not a valid improvement over the current state.

Low

Previous suggestions

Suggestions up to commit 4976e60
CategorySuggestion                                                                                                                                    Impact
Possible issue
Pin sibling to merge commit, not branch head

AGENTS.md §3 invariant 5 states "a bump citing a sibling change must be backed by
the pin" and §4.9 documents that v43 shipped a changelog describing a fix whose pin
never contained it. The kAlgoVersion bump to 51 cites analytics#32, but the pinned
SHA 422c8b8b9631770b07bc571b6fbe904ef59e5052 is explicitly described as a PR-branch
head that is "not yet merged" — if this SHA is force-pushed or the branch is rebased
before the PR lands, the pin silently diverges from what was reviewed. This must be
repinned to the actual merge commit SHA on main before this PR is merged, not after,
to satisfy the invariant.

pubspec.yaml [52-61]

-# PR-BRANCH HEAD, not main — OpenStrap/analytics#32 (fix/issue-170-...)
-# is open, not yet merged. Repin to the merge commit on main once it
-# lands (same pattern v49 used briefly for protocol/analytics PR heads).
-...
-ref: 422c8b8b9631770b07bc571b6fbe904ef59e5052
+# Repin to the merge commit SHA on analytics main once OpenStrap/analytics#32
+# has landed. Do NOT merge this PR with a PR-branch head pinned here.
+ref: <merge-commit-sha-of-analytics#32-on-main>
Suggestion importance[1-10]: 5

__

Why: The suggestion correctly identifies that the ref is pinned to a PR-branch head rather than a merge commit, which could be problematic if the branch is force-pushed. However, the PR itself explicitly acknowledges this as a temporary state with instructions to repin, making this a known and documented trade-off rather than an oversight. The improved_code replaces the SHA with a placeholder, which is not actionable.

Low
Version bump must coincide with verified merge-commit pin

AGENTS.md §3 invariant 5 requires that a version bump citing a sibling change be
backed by the actual pin, verified at the merge commit. The changelog here
explicitly acknowledges the pin is a PR-branch head ("repin to main once merged"),
meaning kAlgoVersion = 51 is being committed while the invariant is knowingly
violated. Existing day_result rows at v50 will not recompute once the correct
merge-commit SHA is repinned, because the version number was already bumped here —
the repin will be a no-op for recomputation. The bump should only be committed
together with the final merge-commit pin, not ahead of it.

lib/compute/derivation_engine.dart [354-365]

 // v51: edge#170 — autoDetectWorkouts' motion-confirmation gate (tuned for
 // arm-swing activities) silently dropped every low-limb-swing cardio window
 // (cycling/rowing: the wrist stays still on a handlebar/oar) no matter how
-// strong the HR signal was. analytics#32 (PR-branch head, pinned above —
-// repin to main once merged) adds an HR-ONSET bypass: the gate is skipped
-// only when mean bpm over the candidate's first 3 min rises >=25 bpm versus
-// the 3 min immediately before it (Whipp & Wasserman 1972 phase-II kinetics),
-// which fires on genuine exercise starts but NOT on slow-drifting elevations
-// (fever/heat/anxiety) that have no discernible onset — so this changes which
-// suggestions autoDetectWorkouts emits without loosening the false-positive
-// gate it exists to protect.
+// strong the HR signal was. analytics#32 adds an HR-ONSET bypass: the gate
+// is skipped only when mean bpm over the candidate's first 3 min rises
+// >=25 bpm versus the 3 min immediately before it (Whipp & Wasserman 1972
+// phase-II kinetics), which fires on genuine exercise starts but NOT on
+// slow-drifting elevations (fever/heat/anxiety) that have no discernible
+// onset — so this changes which suggestions autoDetectWorkouts emits without
+// loosening the false-positive gate it exists to protect.
+// Pinned at analytics merge commit: <merge-commit-sha-of-analytics#32-on-main>
 const int kAlgoVersion = 51;
Suggestion importance[1-10]: 4

__

Why: The suggestion raises a valid concern that kAlgoVersion = 51 is bumped while the analytics pin is still a PR-branch head, but the improved_code only modifies comments and doesn't actually change the version number or resolve the core issue. The PR explicitly documents this as intentional with a repin planned, and the improved_code is nearly identical to existing_code with only minor comment rewording.

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: 1

🤖 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 `@pubspec.yaml`:
- Around line 60-61: Update the openstrap_analytics dependency pin in
pubspec.yaml to a full commit SHA containing the upstream three-minute
onset-window correction, replacing the current
422c8b8b9631770b07bc571b6fbe904ef59e5052 reference. Keep the dependency source
and surrounding configuration unchanged.
🪄 Autofix (Beta)

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: 5a4e4c62-b553-477d-b6e3-e17203cc2ace

📥 Commits

Reviewing files that changed from the base of the PR and between 4cdb065 and 4976e60.

⛔ Files ignored due to path filters (1)
  • pubspec.lock is excluded by !**/*.lock
📒 Files selected for processing (2)
  • lib/compute/derivation_engine.dart
  • pubspec.yaml

Comment thread pubspec.yaml Outdated
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 52b8e9b

@abdulsaheel
abdulsaheel merged commit 5faa4b0 into main Jul 31, 2026
3 checks passed
@abdulsaheel
abdulsaheel deleted the fix/issue-170-cycling-workout-detection branch July 31, 2026 17:27
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.

Auto-detect misses 1h50m road cycling session despite peak HR 157 bpm

1 participant