Skip to content

feat(health): add menstrual cycle tracker#718

Open
Jordan2139 wants to merge 3 commits into
ryanbr:mainfrom
Jordan2139:main
Open

feat(health): add menstrual cycle tracker#718
Jordan2139 wants to merge 3 commits into
ryanbr:mainfrom
Jordan2139:main

Conversation

@Jordan2139

@Jordan2139 Jordan2139 commented Jul 22, 2026

Copy link
Copy Markdown

Store period starts locally and use them to anchor cycle estimates without introducing fertility or medical claims.

Keep Swift and Android storage, UI, and analytics behavior aligned.

What this PR does

Adds an offline menstrual cycle tracker to the Health screen on Apple and Android.

  • Logs period-start dates to the existing metricSeries table under the isolated noop-cycle source.
  • Supports logging today or a previous date, deleting individual entries, and clearing all period history.
  • Feeds logged cycle-day-1 anchors into the existing temperature-based cycle phase engine.
  • Shows the current phase estimate and likely next-period range without fertility, contraception, or medical claims.
  • Keeps all data on-device with no account, telemetry, network request, or cloud synchronization.
  • Adds matching Swift and Kotlin storage behavior, UI flows, analytics wiring, and tests.
  • Adds German, Spanish, and French localization for the new UI.

Type of change

  • Bug fix
  • New feature
  • Refactor / cleanup
  • Documentation
  • CI / tooling

How it was tested

  • Packages/WhoopStore: swift test — 270 tests passed.
    • MetricSeriesStoreTests.testDeletePointUsesFullNaturalKeyAndIsIdempotent verifies physical deletion, source isolation, and idempotency.
  • Packages/StrandDesign: swift test — 32 tests passed.
  • Packages/StrandAnalytics: swift test --filter CyclePhaseEngineTests — 11 tests passed.
    • Covers logged period anchors, period-window estimates, and rejection of anchors that conflict with the temperature pattern.
  • Android: ./gradlew testFullDebugUnitTest — full unit suite passed.
    • CycleTrackingStoreTest covers source/key parity, ISO dates, ordering, idempotent logging, and deletion.
    • CyclePhaseEngineTest covers logged-anchor behavior and period-window estimates.
  • Android: ./gradlew compileFullDebugKotlin passed.
  • macOS: xcodebuild -project Strand.xcodeproj -scheme Strand -destination 'platform=macOS' CODE_SIGNING_ALLOWED=NO build passed.
  • python3 tools/i18n_audit.py --ci main passed for Apple and Android.
  • git diff --check passed.
  • No BLE behavior changed, so strap hardware testing was not required.
  • NOOPiOS: generic iOS device build passed, including the embedded widgets, Watch app, and complications.

Checklist

  • Swift package tests pass for any package I touched (swift test in Packages/<name>)
  • Android unit tests pass if I touched android/ (./gradlew testFullDebugUnitTest)
  • No new build warnings introduced
  • UI changes use only StrandDesign tokens — no hardcoded colors, fonts, or spacing
  • No hardcoded hex frame bytes; protocol facts live in the schema / decoders
  • Follows the conventions in docs/CONTRIBUTING.md
  • I did not commit generated output (Strand.xcodeproj/) or any secrets/keystores

Related issues

None.

Store period starts locally and use them to anchor cycle estimates without introducing fertility or medical claims.

Keep Swift and Android storage, UI, and analytics behavior aligned.
@ryanbr

ryanbr commented Jul 22, 2026

Copy link
Copy Markdown
Owner

Review

Good feature — well-structured, cross-platform parity, good test coverage, and careful scoping (no fertility/medical claims, data stays local). A few things to address before merging:

Blockers

  1. Android phase strings are hardcoded EnglishcycleTrackerPhase() in CycleTrackerDialog.kt returns "Follicular", "Mid-cycle shift", etc. instead of referencing strings.xml. The rest of the dialog correctly uses uiString(R.string.*) — these should too, especially since DE/ES/FR translations are already in the PR for everything else.

  2. CycleAwarenessCard wiring — the PR passes onLogPeriod and onOpenDetail to CycleAwarenessCard on both platforms, but the diff doesn't show the card's own definition accepting those new parameters. Can you confirm the card body is updated to wire them through?

Non-blocking (follow-up)

  1. deleteAllPeriodStarts is N individual deletes — both Swift and Kotlin loop over each start and delete one by one. A single DELETE FROM metricSeries WHERE deviceId = ? AND key = ? would be atomic and faster. Fine as a follow-up.

Nits

  • prettyDay() is called in CycleTrackerView (Swift) but its definition isn't visible in the diff — presumably it exists elsewhere, just worth confirming.
  • The CyclePhaseEngine.classify signature change (adding loggedPeriodStarts) isn't shown — assuming it's already landed or in a companion change.

Everything else looks solid: source isolation (noop-cycle), idempotent upsert/delete, test coverage on both platforms, design tokens only, localization for 3 languages.

@Jordan2139

Copy link
Copy Markdown
Author

Thanks for the review — the requested changes and confirmations are now on the PR branch.

  • Android phase localization: Replaced the hardcoded English values in cycleTrackerPhase() with uiString(R.string.*) lookups in both CycleTrackerDialog.kt and SkinTempCardsScreen.kt. Added the five phase labels to the EN, DE, ES, and FR resources.
  • CycleAwarenessCard wiring: Confirmed on both platforms. The Swift card accepts optional onLogPeriod and onOpenDetail closures and invokes them from its actions. The Compose card accepts the equivalent callbacks and wires them to its buttons. The Home/Today card now passes those callbacks through so users can log a start or open cycle detail directly.
  • prettyDay(): Confirmed in SkinTempCardsView.swift; it is shared by the card/detail presentation code.
  • CyclePhaseEngine.classify: Confirmed that loggedPeriodStarts is part of the Swift and Kotlin implementations included in the feature change.

Verification completed:

  • ./gradlew testFullDebugUnitTest
  • Android localization audit
  • NOOPiOS app-target build
  • Strand macOS app-target build

No BLE path was changed, so strap hardware testing is not applicable. The focused follow-up is in 2cacea0c; the Home card wiring is in 0ee3b928.

@ryanbr

ryanbr commented Jul 25, 2026

Copy link
Copy Markdown
Owner

Re-reviewed. Both blockers are fixed — verified on the branch rather than taken on trust.

1. Android phase localizationcycleTrackerPhase() at CycleTrackerDialog.kt:251-257 now returns uiString(com.noop.R.string.cycle_phase_*) for all five cases, and each of values, values-de, values-es, values-fr carries six cycle_phase_* entries. The cycle_phase_* naming does not follow the dominant l10n_<screen>_<hash> convention, but 207 hand-named strings already exist alongside 1491 generated ones, so that is fine as-is.

2. CycleAwarenessCard wiring — confirmed on both platforms: SkinTempCardsView.swift:67,69 declares the optional closures and invokes them at :178-184, with HealthView.swift:1339,1345 passing them; SkinTempCardsScreen.kt:120-121 and :172-174 mirror it, wired from HealthScreen.kt:453,475-476.

Better than you claimed on localization. The PR notes say German, Spanish and French. The new Swift strings actually carry eight locales — de, es, fr, it, pt-PT, ru, zh-Hans, zh-Hant — including Log period start, View detail, and the phase labels. Worth stating accurately; it is more coverage than the description promises.

The conflict is trivial

CONFLICTING/DIRTY looks worse than it is. It is exactly one hunk in each of the four Android strings.xml, and the two sides add entirely different strings — settings_streak_* arrived on main while this branch added cycle_tracker_* / cycle_home_*. No string is touched by both. The resolution is "keep both sides", then re-run. You are 86 commits behind main, so a rebase is worth doing anyway.

No CI has run on this branch yet, presumably because of the conflict. The i18n audit is the gate that matters here given ~40 new strings across four Android locales and eight iOS ones, so please confirm it goes green after the rebase.

One new thing, and it is the one worth discussing

The shipped copy reads:

This stays on your device. It is never uploaded, never synced, never shared.

The first two clauses are exactly right — there is no network path, no telemetry, and no platform-health export (I checked: nothing in the Health Connect / HealthKit paths references cycle data). But "never shared" is doing more work than the code supports, in two user-initiated paths:

  • .noopbak is a whole-database copy. The table list at DataBackup.kt:208 is a validation read for rejecting foreign backups, not a filter on what gets carried — so a backup includes metricSeries, and therefore the period history.
  • The CSV/JSON data export serialises metricSeries rows (WhoopCsvExporter.metricSeriesJson), keyed by deviceId/day/key, so noop-cycle rows travel with everything else.

Nothing covert is happening, and for heart-rate samples I would not raise it. For menstrual data the reading matters: someone who exports a backup to attach to a bug report here, or hands a CSV to a coach, would be sharing their period history without having been told. That is a decision for @ryanbr rather than you, and either resolution is defensible:

  • soften the copy so it names the export caveat ("never leaves your device unless you export it yourself"), or
  • exclude the noop-cycle source from the export and backup paths, and keep the absolute claim.

I would lean to the first, since silently dropping a source from a backup creates its own surprise on restore.

Still outstanding from the first review

deleteAllPeriodStarts remains N individual deletes on both platforms. Non-blocking, as before.

Otherwise this is good work — the source isolation under noop-cycle, the idempotent upsert/delete with tests on both platforms, the deliberate absence of fertility or medical framing, and design-token-only UI are all right.

@ryanbr

ryanbr commented Jul 25, 2026

Copy link
Copy Markdown
Owner

Correction to my previous comment — I got half of the privacy point wrong, in the direction that made your design look worse than it is.

I said the cycle data travels in two user-initiated paths. It is one.

Wrong: the CSV/JSON export. I claimed noop_metric_series.json carries the period history. It does not. The sidecar is built from repo.importedSourceIds(deviceId) + repo.computedSourceIds(deviceId) (WhoopCsvExporter.kt:347-348, 390-394) — strap-derived source ids for the active device. Your data lives under the dedicated noop-cycle SOURCE_ID, which is outside that set, so it is excluded from the export.

That is not luck, it is the isolation you built on purpose — CycleTrackingStore's own comment says imports and strap analysis use different source ids so they cannot overwrite user-entered history. The same boundary keeps period starts out of a shared CSV. I turned a deliberate design property into a criticism, which was the opposite of accurate.

Still correct: the .noopbak backup. DataBackup.exportTo copies the entire SQLite file byte-for-byte into the zip (DataBackup.kt:81, 128dbFile.inputStream().use { input -> input.copyTo(zip) }), and the class comment says NOOP keeps everything in that single file. So a backup does carry metricSeries, and therefore the period history.

So the accurate picture is narrower and much better than I first described:

Path Carries cycle data?
Network / telemetry / sync No — none exists
Health Connect / HealthKit No
CSV + JSON data export Nonoop-cycle is outside the exporter's source scope
.noopbak backup Yes — whole-file copy

The remaining question is the same but smaller: "never shared" is absolute, and someone attaching a .noopbak to a bug report here would be including their period history without having been told. One path, well defined, and arguably worth one clause of copy rather than a code change. @ryanbr's call.

Apologies for the noise — I should have traced sidecarRows to its source before posting the first version rather than after.

@ryanbr

ryanbr commented Jul 25, 2026

Copy link
Copy Markdown
Owner

Two things left. Rebase is already done on my side — the 4 conflicts are append-only in the Android strings.xml (settings_streak_* from main vs your cycle_tracker_*), resolved by union, your authorship intact on all three commits. Say the word and I'll push it to your branch or send the patch.

1. pt-PT — 55 keys. The gate fails on this now. Worth saying: pt-PT became a gated locale after you ran the audit, so this is branch staleness rather than something you missed.

  • values-pt-rPT/strings.xml — 32 keys: cycle_home_description, cycle_home_latest, cycle_home_learning, cycle_home_log_today, cycle_home_logged_today, cycle_home_open, cycle_home_private, cycle_home_setup, cycle_home_title, cycle_phase_follicular, cycle_phase_learning, cycle_phase_luteal, cycle_phase_mid_cycle, cycle_phase_unclear, cycle_tracker_already_logged, cycle_tracker_anchor_help, cycle_tracker_cancel, cycle_tracker_current_estimate, cycle_tracker_day_range, cycle_tracker_day_single, cycle_tracker_delete_all, cycle_tracker_delete_day, cycle_tracker_delete_message, cycle_tracker_delete_title, cycle_tracker_done, cycle_tracker_empty, cycle_tracker_likely_window, cycle_tracker_log_action, cycle_tracker_log_heading, cycle_tracker_logged_starts
  • Strand/Resources/Localizable.xcstrings — the same 23 English keys

The "format mismatch" errors (4 Android, 3 iOS) are the same problem, not extra work — a missing translation reads as an empty value and fails the placeholder check. Verified: adding one entry dropped missing 23→22 and mismatches 3→2 together. Keep %1$d / %lld / %@ intact and both clear.

2. Privacy copy. .noopbak is a whole-file DB copy, so a backup a user shares contains their period history. Decision is to keep it in backups — it should persist like any other health data — and adjust the wording instead. Something like "never leaves your device unless you export a backup yourself".

Cross-platform portability (CSV route carries no period starts) is a separate follow-up, not this PR. deleteAllPeriodStarts N-deletes still non-blocking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants