Repository navigation
Refactor Today pure logic helpers - #686
Conversation
ryanbr
left a comment
ryanbr
left a comment
There was a problem hiding this comment.
Good direction — this is the #665 Phase-1 split the maintainer outlined, and the logic move is faithful. Verified three ways:
- 66 symbols removed = 66 added, identical signatures (only change is
com.noop.analytics.FusionSource→ importedFusionSource). Nothing dropped or duplicated. - Real code lines move ~1:1 (≈332 → ≈326 after excluding comments/blanks/imports).
- Compiles + the moved-helper tests pass (DayNav, RecoveryCalibration, HrWindow, LastWorkoutsFeed, TodayMetricTiles, StepsReadingsMerge, TodayExplainability).
One thing to fix before merge: ~223 lines of comments/KDoc got dropped (298 comment lines removed vs 75 added — that's the whole 688-vs-479 gap), and they're the valuable kind:
- 26 issue-provenance refs gone (30
#NNNcomment lines removed, 4 kept): the "why it exists / which bug motivated it" trail —#547future-dated-row guard,#543/#779carry-over + freshness cap,#860"as ONE pure decision so the rule can't drift",#393/#85baseline calibration. - The cross-platform parity anchors are stripped — "Mirror EXACTLY in Swift", "Mirrors the iOS lastScoredCharge tuple", "Mirrors iOS lastChargeDateFmt". Those guard the repo's #1 rule (Swift↔Kotlin parity); a future edit to the now-bare extracted helper won't see them and could silently diverge from the Swift twin.
Could you carry the KDoc/rationale comments over with the functions? A pure move should move the comments too — especially the "Mirror EXACTLY in Swift" / "Mirrors iOS…" parity notes and the #NNN provenance. The logic is sound; this is purely about not losing the context that keeps these helpers maintainable and parity-safe. With the comments preserved, happy to merge.
|
Pushed follow-up commit a2f1ffb to address this review. The extraction now carries the Today helper KDoc/rationale comments, issue provenance refs, and Swift/iOS parity notes over into the new helper files without changing the helper logic.\n\nVerified locally with:\n- |
…xtraction Refactor Today pure logic helpers
…xtraction Refactor Today pure logic helpers
…ple counts RecoveryScorer.restingHR and RecoveryScorer.recoveryIndexSlope document and prefilter a closed [start, end] window but bin it as [t, t + 300) with a loop that stops at `end`. A sample sitting exactly on an aligned `end` passes the prefilter and then belongs to no bin: admitted as data, silently ignored. For the Recovery Index that can leave the six-bin gate one bin short and turn a complete 30-minute window into nil/null. The final bin now closes on `end` on both platforms ([t, t + 300) for every bin except the last, which is [t, end]); a zero-length window collapses to that one final bin. Bin midpoints, the #686 artifact gate and recoveryIndexMinBins are unchanged, so nothing moves except a sample on an aligned endpoint. Both functions already guard on an empty prefiltered segment, so the loop conversion cannot manufacture a bin. Doc comments and docs/ANALYTICS.md state the rule. Verified surgical rather than taken on trust: sweeping both the old and new binning over 55,100 random window/sample configurations produced zero differences for any span not aligned to 300s, and zero differences that did not involve a sample sitting exactly on `end`. The tests earn their place. Reverting only the production Kotlin file turns three of the ten new cases red (endpointSampleLandsInFinalBinOnly, recoveryIndexSlopeAlignedEndpointCompletesTheBinGate, restingHrEndpointSweepMatchesSwiftOracle) and they pass again with it; the other seven pin behaviour that must not move. Scope, stated plainly: both helpers are library API with no in-app caller today, so this moves no shipped number. The resting HR the apps display comes from SleepStager.sessionRestingHR, whose window loop has the same shape and the same defect; that is deliberately not part of this change. Reported and implemented by @bhelm. The reporting fork's issue number was dropped from the comments before merge: read in this repo it lands on an unrelated report, and the same number is already used in HealthLiveHrLogic.kt and AppViewModel.kt for live-HR median parity.
Summary
Follow-up to #665. This starts the low-risk Phase 1 split the maintainer outlined for
TodayScreen.kt: move pure/package-level Today helpers into sibling files in the same package, without changing call sites or behavior.Extracted files:
TodayDayNav.kt— launch/day navigation helpers.TodayScoring.kt— recovery/scoring/recording state helpers.TodayProvenance.kt— source/provenance/readiness/footer summary helpers.TodayMetricsLogic.kt— HR window, footer cache shape, workout feed, weight/steps tile helpers.TodayScreen.ktdrops 688 lines and remains the UI assembler. No iOS twin is included because this is Android/Kotlin file organization only.Verification
JAVA_HOME=/opt/homebrew/opt/openjdk@21/libexec/openjdk.jdk/Contents/Home ANDROID_HOME=$HOME/Library/Android/sdk ./gradlew compileFullDebugKotlin testFullDebugUnitTest --tests com.noop.ui.DayNavTest --tests com.noop.ui.TodayLaunchLandTest --tests com.noop.ui.LandTargetDayStartTest --tests com.noop.ui.RecoveryCalibrationTest --tests com.noop.ui.TodayExplainabilityTest --tests com.noop.ui.TodayChargeTapCollapseTest --tests com.noop.ui.TodayResolverEffortScaleTest --tests com.noop.ui.HrWindowTest --tests com.noop.ui.LastWorkoutsFeedTest --tests com.noop.ui.TodayMetricTilesTest --tests com.noop.ui.StepsReadingsMergeTestpython3 Tools/i18n_audit.py --ci origin/main