Multiple fixes - #111
Merged
Merged
Conversation
Readiness's RHR input fell back to a few minutes of live daytime HR (via the rhr metric's own dayHrValid fallback) whenever no sleep had been detected yet, while HRV/resp/temp already correctly required a real sleep session. That asymmetry let a no-sleep day still produce a full numeric readiness score off RHR alone (e.g. "Readiness 100" ~10 min after first wearing the strap). rhrToday now requires hasSleep && sleepHr.isNotEmpty. Also removed two copies of a "100 - readiness" stress fallback (getDayStress and stressSummaryForToday) that fabricated a stress-looking number whenever the real Baevsky SI was absent, violating the never-impute rule. Both now correctly return null/"-". kAlgoVersion 46 -> 47 so affected days re-derive.
- Today screen "raw data collected" state now reads "Data from <date> is being collected" plus a real collection-progress bar (last ingested record ts vs now), instead of a bare, meaningless raw count. - AI morning-briefing card is now hidden entirely with no BYOK key configured, instead of always showing a placeholder greeting that implies a briefing will eventually appear regardless. - Rhythm section in the Heart screen renamed from "Rhythm screen" to "Rhythm" — it's a section, not its own route. - ListRow's `value` text (e.g. companion URL) now wraps in Flexible + ellipsis instead of being able to overflow the row unbounded. - Step goal default unified to 8000 (StepGoalScreen.defaultGoal) everywhere a fallback existed; several call sites previously fell back to a stale 10000. - Calories/steps split out of the "Training load" section on the Strain/Body screen into their own "Calories & steps" section — training load (ACWR/fitness trend/effort) and energy expenditure are different concepts. - Workout finish/hold-to-end control panel now adds the system gesture-nav bottom inset instead of a fixed offset, so it can't sit under the Android nav bar. - AppState.deleteWorkout now clears `activeWorkout` when the deleted session is the one currently tracked live, fixing a stale "Run live" state after deleting a just-finished manual workout.
…file
Today's lookback card and the Heart screen's day-detail section each
hand-rolled their own copy of the HR TimeSeriesChart + peak/low/now
chip row (chips above vs below, cutoff-to-now or not, and a manually
reimplemented tooltip that happened to exactly match TimeSeriesChart's
own default formatter already) — three near-identical HR charts
drifting apart across the app. Extracted the shared HrCurveWithChips
widget (lib/ui/kit/charts.dart) with chipsPosition/cutoffToNow/
showNowChip params; both screens now call it. (The third "HR graph"
mentioned in feedback — the generic multi-metric trend/bar screen — is
intentionally shared infra every metric drills through, not an HR
duplicate, so it's untouched.)
Also added two AI entries under Profile ("Briefings & journal" and
"AI coach") — the agentic AI Coach chat was previously reachable only
via a small pill button on the Body screen, with no path to it from
Profile/Settings at all.
google-services.json / GoogleService-Info.plist are gitignored with no
committed placeholder, but the Android Gradle google-services plugin
was applied unconditionally and the iOS Xcode project statically
referenced GoogleService-Info.plist in its Resources copy phase — a
from-scratch clone with no Firebase project configured couldn't run
`flutter build apk` or `flutter build ios` at all.
- Android: the three Firebase Gradle plugins (google-services,
firebase-perf, crashlytics) now apply only `if
(file("google-services.json").exists())`; the release
CrashlyticsExtension config is gated the same way (configuring an
extension from a plugin that was never applied would otherwise fail
Gradle's configuration phase even with the plugin skipped).
- iOS: a new "Ensure GoogleService-Info.plist" Run Script build phase
(ordered before Resources) generates a format-valid, inert,
never-committed placeholder plist only if one isn't already present
— never overwrites a real file a contributor placed there for their
own Firebase project.
Verified end-to-end: with both files absent, `flutter build apk
--debug` and `flutter build ios --debug --no-codesign --simulator`
both succeed. No Firebase telemetry (Crashlytics/Analytics/
Performance) without a real project configured — that's the intended
trade for a repo that just builds. firebase_options.dart's existing
dummy stays as-is (Dart-side init was already safe; this closes the
native-side gap).
…pp Store/Play Store always zero-collection Reframes the privacy model by release channel instead of a single blanket "we never collect anything" for every build we distribute: - Official App Store/Play Store releases: unchanged, zero health-data collection, ever. - GitHub Releases (sideload APK/IPA): health-data-contribution (kHealthDataContributionEnabled) is now compiled in, but stays opt-in per user via the existing "Contribute my health data" toggle in onboarding/Settings, which defaults off and is itself hidden whenever the build-time flag is false. No behavior change to the actual upload gating (HealthUploader.maybeUpload already re-checks the flag directly) — only the compile-time flag's value for the GitHub Releases CI job. Fixes found while reviewing the in-flight change: - The ios build job still had the OLD value/comment (ENABLE_HEALTH_DATA_CONTRIBUTION=false, "matching PRIVACY.md's we do not collect your data") while the android job had already moved to the new policy — brought ios to parity so both GitHub-release platforms behave the same. - kHealthDataContributionEnabled's doc comment in health_uploader.dart still described the old "not enabled by ANY build" model; updated to describe the actual two-tier behavior. - Root PRIVACY.md (unlinked from the app, but the plain-text copy anyone browsing the repo on GitHub sees) was still describing the old policy verbatim, two days stale relative to docs/privacy.html (the one the app actually links to). Brought both into sync, including the "Last updated" date. Verified the code already matches what the new policy promises: kHealthDataContributionEnabled gates both the upload path and the Settings/onboarding toggle's visibility; the toggle itself defaults off (profile_setup_screen.dart); maybeUpload() re-checks the flag directly as defense in depth. No functional/runtime change beyond the ios CI flag flip.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe changes update release-channel Firebase and health-data configuration, clarify privacy disclosures, correct readiness and stress derivation behavior, and revise UI flows for charts, onboarding progress, activity metrics, step goals, settings, consent, and workout deletion. ChangesRelease configuration and privacy policy
Health derivation and data integrity
Application UI and state flows
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 `@android/app/build.gradle.kts`:
- Around line 6-28: The Firebase plugin application guard must not treat an
empty google-services.json as configured. Update the conditional around the
apply(plugin = ...) calls to require a non-empty, valid configuration file, or
ensure the CI workflow only writes the file when Firebase secrets are present;
preserve the optional Firebase behavior for missing or empty configuration.
In `@lib/data/db.dart`:
- Around line 2192-2206: Update firstAndLastRecordTs() to aggregate only
decoded_onehz rows with rec_ts > 0, matching the filtering used by rawStats()
and lastDecodedRecTs(). Preserve the existing null tuple behavior when no
qualifying rows exist.
In `@lib/state/app_state.dart`:
- Around line 3113-3118: Update deleteWorkout to use a dedicated non-persisting
active-workout cancellation path before deleting the repository row. That path
should stop and clear _workoutTimer, stop and clear _routeTracker, end the Live
Activity, and reset the active workout state without finalizing or persisting a
session; invoke it when activeWorkout.workoutId matches id, then retain the
existing deletion and notification flow.
In `@lib/telemetry/health_uploader.dart`:
- Around line 24-41: Make health-data contribution genuinely opt-in by
initializing the Profile consent setting healthShareConsent to false and
updating its UI copy to describe the default-off behavior. In
lib/telemetry/health_uploader.dart lines 24-41, preserve the documented
default-off contract; align the corresponding policy language in PRIVACY.md
lines 24-27 and docs/privacy.html lines 44-50 with the corrected consent
behavior.
In `@lib/ui/screens/screens.dart`:
- Line 315: Update the goal fallback in the surrounding StepGoalScreen logic to
use StepGoalScreen.defaultGoal whenever goal is null or non-positive, including
zero, matching the established behavior in TodayScreen. Preserve positive goal
values unchanged so progress calculations never divide by zero.
In `@lib/ui/today/today_screen.dart`:
- Line 941: Align the local repository defaults with StepGoalScreen.defaultGoal
by updating getProfile() and _stepGoal() in LocalRepositoryImpl to use the
shared default instead of 10000, while retaining the existing Today screen
fallback.
- Around line 494-514: Cache the Future returned by
LocalDb.firstAndLastRecordTs() in the Today screen state instead of constructing
it inline in the FutureBuilder. Initialize or refresh that cached future only
when raw changes, and have the FutureBuilder use the cached future so unrelated
rebuilds preserve its snapshot and avoid redundant queries.
In `@lib/ui/workouts/workouts_screen.dart`:
- Around line 908-911: Update the detail deletion flow around app.deleteWorkout
to return a deletion result when the workout is successfully removed, and have
WorkoutsScreen await the detail route result and reload its list when deletion
is reported. Preserve the existing behavior for cancellations and failed
deletions, using the route result as the refresh signal.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e44e2cf3-3169-43b5-a05b-8669978f8751
📒 Files selected for processing (23)
.github/workflows/build.ymlPRIVACY.mdandroid/app/build.gradle.ktsdocs/privacy.htmlios/Runner.xcodeproj/project.pbxprojlib/compute/derivation_engine.dartlib/compute/onehz_pipeline.dartlib/data/db.dartlib/data/local_repository_impl.dartlib/state/app_state.dartlib/telemetry/health_uploader.dartlib/ui/activity/live_session_screen.dartlib/ui/activity/strain_detail_screen.dartlib/ui/design/rows.dartlib/ui/kit/charts.dartlib/ui/kit/state_card.dartlib/ui/profile/profile_screen.dartlib/ui/screens/detail_cards.dartlib/ui/screens/screens.dartlib/ui/today/today_screen.dartlib/ui/workouts/workouts_screen.darttest/week_view_feed_test.darttest/workout_enrichment_test.dart
Comment on lines
+3113
to
+3118
| Future<void> deleteWorkout(String id) async { | ||
| await repo?.deleteWorkout(id); | ||
| if (activeWorkout?.workoutId == id) { | ||
| activeWorkout = null; | ||
| _workoutRawBase = null; | ||
| notifyListeners(); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Fully tear down an active workout before deleting it.
This clears only the in-memory flags. _workoutTimer, _routeTracker, and the Live Activity remain active; the route tracker can continue appending GPS points for the deleted workout ID. Add a dedicated non-persisting cancellation path that stops the timer/tracker and ends the Live Activity before removing the row. Do not call stopWorkout() blindly, because it first persists a finalized session.
🤖 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 3113 - 3118, Update deleteWorkout to
use a dedicated non-persisting active-workout cancellation path before deleting
the repository row. That path should stop and clear _workoutTimer, stop and
clear _routeTracker, end the Live Activity, and reset the active workout state
without finalizing or persisting a session; invoke it when
activeWorkout.workoutId matches id, then retain the existing deletion and
notification flow.
Comment on lines
+24
to
+41
| /// Two-tier by release channel (see docs/privacy.html's "GitHub releases" | ||
| /// section, the user-facing policy): OFF for any official App Store/Play | ||
| /// Store submission (once that pipeline exists it must pass | ||
| /// `ENABLE_HEALTH_DATA_CONTRIBUTION=false` explicitly), ON for the GitHub | ||
| /// Releases build (.github/workflows/build.yml, both android + ios jobs) — | ||
| /// but "ON" here only means the feature EXISTS in that binary; actual | ||
| /// upload additionally requires the user to explicitly flip the in-app | ||
| /// toggle, which defaults off and is itself hidden whenever this flag is | ||
| /// false. Uploading someone's entire raw + derived health history to a | ||
| /// backend is by far the biggest privacy/compliance surface this app could | ||
| /// have, so keep the App Store/Play Store channel's "we do not collect | ||
| /// your health data" promise (docs/privacy.html) true by construction of | ||
| /// the CI config, not contingent on nobody flipping this default later. | ||
| /// | ||
| /// This flag exists purely because the code is open source: an independent | ||
| /// developer compiling their OWN build from this repo can opt in and point | ||
| /// it at a backend of their own choosing. That is their build and their | ||
| /// responsibility, not something our privacy policy governs. | ||
| /// developer compiling their OWN build from this repo can also opt in and | ||
| /// point it at a backend of their own choosing. That is their build and | ||
| /// their responsibility, not something our privacy policy governs. |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Make health-data contribution genuinely opt-in before claiming it is.
The supplied Profile toggle says “On by default,” while these changes promise explicit consent and a default-off setting. This can permit full local health-database uploads after Wi-Fi/charging checks without the documented opt-in. Initialize healthShareConsent to false and update the UI copy, or revise all disclosures to describe the actual behavior.
lib/telemetry/health_uploader.dart#L24-L41: keep the default-off contract only after the consent default is corrected.PRIVACY.md#L24-L27: align the policy with the corrected consent behavior.docs/privacy.html#L44-L50: mirror the corrected policy language.
📍 Affects 3 files
lib/telemetry/health_uploader.dart#L24-L41(this comment)PRIVACY.md#L24-L27docs/privacy.html#L44-L50
🤖 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/telemetry/health_uploader.dart` around lines 24 - 41, Make health-data
contribution genuinely opt-in by initializing the Profile consent setting
healthShareConsent to false and updating its UI copy to describe the default-off
behavior. In lib/telemetry/health_uploader.dart lines 24-41, preserve the
documented default-off contract; align the corresponding policy language in
PRIVACY.md lines 24-27 and docs/privacy.html lines 44-50 with the corrected
consent behavior.
- android/build.gradle.kts + ios pbxproj: the Firebase-optional guard checked file existence only. CI writes google-services.json / GoogleService-Info.plist unconditionally from a secret (base64 -d > file), so an unset/blank secret produced a 0-byte file that still passed the "is Firebase configured" check and then failed the google-services plugin / Xcode resource copy trying to parse it. Both sides now also require the file to be non-empty. - db.dart: firstAndLastRecordTs() was missing the `rec_ts > 0` filter its sibling queries (rawStats/lastDecodedRecTs) already have — a stray rec_ts=0 row would have rendered "Data from Jan 1" (1970 epoch) in the onboarding progress card. - app_state.dart: deleteWorkout only cleared activeWorkout in-memory; if the deleted session was GENUINELY still live, its timer, GPS route tracker, and Live Activity kept running against a now-deleted id. New _cancelActiveWorkoutTeardown() stops all three without persisting (unlike stopWorkout, which finalizes a session). - workouts_screen.dart: deleting a workout from the detail screen popped without signaling the list screen (which doesn't observe AppState), leaving the deleted row visible until a manual refresh. Detail now pop(true)s on delete; the list awaits the push and reloads on that signal. - profile_setup_screen.dart (real, more serious than reported): `_healthShare` and `ProfileSetupForm.healthShareInitial` both defaulted to `kHealthDataContributionEnabled` itself — meaning on any build compiling the feature in (GitHub releases, per the prior commit's new policy), the "Contribute my health data" toggle was PRE-CHECKED for every fresh enrollment, not off-by-default as promised. The flag must gate only whether the toggle is offered, never its starting value. Both now default to `false` unconditionally; a returning user's own previously-recorded choice is still honored. Updated the test that had encoded the buggy default as expected. - screens.dart: the `goal ?? StepGoalScreen.defaultGoal` fallback didn't guard `goal == 0`/negative (division-by-zero / misleading-progress risk), inconsistent with today_screen.dart's `stepWeekRingData` which already guards `goal > 0`. - Actual root cause of the step-goal-default miss: local_repository_impl.dart's getProfile()/_stepGoal() still hardcoded 10000 — that's where TodayData.stepGoal gets its default BEFORE any UI-level `?? 8000` fallback ever sees a null, so the UI fallback from the previous commit never actually fired for real profile data. Introduced kDefaultStepGoal (lib/data/local_repository.dart) as the single source of truth; StepGoalScreen.defaultGoal now just aliases it. Verified: full test suite green (489 tests), flutter analyze clean, and the two build-tooling fixes validated by actually building with a 0-byte google-services.json (fails before this fix, succeeds after).
Contributor
There was a problem hiding this comment.
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 `@lib/ui/workouts/workouts_screen.dart`:
- Around line 377-387: Guard the post-navigation refresh in the
WorkoutDetailScreen onTap callback by checking mounted before calling _load()
when deleted is true. Preserve the existing deletion signal and avoid invoking
_load() after WorkoutsScreen has been unmounted.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1d6541ba-c51f-4c4e-85f0-e051751ad1cd
📒 Files selected for processing (12)
android/app/build.gradle.ktsios/Runner.xcodeproj/project.pbxprojlib/data/db.dartlib/data/local_repository.dartlib/data/local_repository_impl.dartlib/state/app_state.dartlib/ui/profile_setup_screen.dartlib/ui/screens/screens.dartlib/ui/today/step_goal_screen.dartlib/ui/today/today_screen.dartlib/ui/workouts/workouts_screen.darttest/flow_screens_redesign_test.dart
🚧 Files skipped from review as they are similar to previous changes (6)
- lib/state/app_state.dart
- ios/Runner.xcodeproj/project.pbxproj
- lib/ui/screens/screens.dart
- lib/data/db.dart
- lib/data/local_repository_impl.dart
- lib/ui/today/today_screen.dart
New CodeRabbit finding on PR #111 (posted after the previous fix commit): the onTap handler awaits the detail-screen push and then calls _load() (which calls setState) if a deletion was signaled — but WorkoutsScreen itself could be unmounted by the time that await resolves. Added the mounted guard.
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.
This pull request introduces several important privacy policy clarifications, build system improvements, and telemetry configuration changes. The privacy documentation now distinguishes between official App Store/Play Store releases and GitHub releases, clarifying when health data contribution and anonymous diagnostics are enabled. The build system for both Android and iOS is updated to make Firebase telemetry (Crashlytics, Analytics, Performance) optional, ensuring the app builds and runs even without Firebase credentials. Additionally, the algorithm version is bumped to address readiness and stress score calculation logic.
Privacy policy and documentation updates:
PRIVACY.mdanddocs/privacy.htmlnow clearly states that health data is not collected in official App Store/Play Store releases, while GitHub releases may allow opt-in health data contribution for algorithm improvement. Anonymous diagnostics are on by default only in GitHub releases and can be disabled by the user. [1] [2] [3] [4]Build system and telemetry configuration:
apply falseand only conditionally applied ifgoogle-services.jsonis present, allowing builds to succeed without any Firebase credentials. Crashlytics configuration is also conditional. [1] [2]GoogleService-Info.plistif none is present, ensuring builds do not fail on a fresh clone without Firebase credentials. [1] [2]CI configuration and health data contribution:
.github/workflows/build.yml: Health data contribution is now intentionally enabled (ENABLE_HEALTH_DATA_CONTRIBUTION=true) for GitHub release builds (both Android and iOS), matching the clarified privacy policy. The comments explain the rationale and user opt-in nature. [1] [2]Algorithm versioning:
lib/compute/derivation_engine.dart: BumpskAlgoVersionto 47, updating readiness score logic to require actual detected sleep sessions and removing fallback logic for stress scores, ensuring more accurate and policy-compliant metrics.Summary by CodeRabbit
New Features
Bug Fixes
Documentation