Repository navigation
store: keep the nightly absolute skin temperature - #1663
Merged
Merged
Conversation
The engine computed the wear-gated nightly mean on every scoring pass, took the deviation from it, and threw the absolute away. So the app could show "+0.5 delta-C" with no way anywhere to learn what it moved from -- and a febrile night reads as a small delta where the absolute reads as a fever. Reported against #1636. This is the persistence half only. No decoding, no new math: the value is the one wornNightlySkinTempC already produces, written at the same line the deviation it derives is written, so the two can never describe different nights and no second derivation exists to drift. - GRDB v40 / Room v34: dailyMetric.skinTempC, nullable and additive - written in AnalyticsEngine beside skinTempDevC so the engine's own row is symmetric, and again on the scored row in IntelligenceEngine - carried through every path that REBUILDS a row: coalesceDay, mergeDaily, the CSV importer fill, both Swift with() helpers. Each spells its fields out by name, so a new column is dropped by omission, not by a compile error -- a value that persists correctly and then vanishes on the way to being read Deliberately NOT a backfill. Existing nights refill on the next scoring pass because the value is re-derived from raw skinTempSample rows still on disk. A migration-time recompute would be a SECOND implementation of the same mean, free to disagree with the live one, and would have to reproduce the per-family raw conversion (#938) for hardware I cannot test. The new column is unambiguous, which skinTempDevC is not: CSV/Health imports write an absolute wrist temperature into that column and SkinTempDisplay separates them by magnitude. Only the strap pipeline writes this one. Two repo guards fired and both were right. The shared Room<->GRDB schema oracle needed the version, the migration id and the column (both copies). And analyzeRecentOnCpu sits within ~4 KB of the 64 KB JVM method ceiling once JaCoCo instruments it; one more named copy argument put it 25 bytes over, so the row assembly is extracted -- it now measures 61518 against a 61535 budget. Verification: 7 new tests per platform (migration shape, round trip, upsert correction, and every rebuild path). Android 4510 tests green, compileFullDebugKotlin clean, doc_comment_lint and i18n_audit clean. Surfacing it is a separate concern and a separate PR, per the one-concern rule. Landing the column first means history is already populated by the time any UI reads it.
The Kotlin suite covered mergeDaily and the Swift one did not, so the cross-bucket merge -- the path a user with any WHOOP-export history actually goes through -- was guarded on one platform only. Added. Also names the two asymmetries rather than leaving the counts to imply parity that is not there: the with() rebuild helpers are a Swift-only shape (Kotlin's data-class copy carries every field, so there is no seam to drop a column at), and the store round-trip is Swift-only because this environment has no Robolectric, which AppleStepHourMigrationTest already documents.
app-build caught it: the two with(...) helpers live in a `private extension`, so the carry test could not compile against them. DailySkinTempAbsoluteCarryTests.swift:48:29: error: 'with' is inaccessible due to 'fileprivate' protection level @testable grants access to `internal`, not `fileprivate`, and I assumed the extension was the former. Those two rebuilds are exactly the seams a new column is dropped at -- they respell every field by name -- so the test should assert them directly rather than a copy that could drift. The extension is now internal; nothing outside the module can see it either way. Also declares `throws` on the test methods that use `try XCTUnwrap`. The XCTAssert autoclosure is throwing so it compiled without, but this is the one file in the change I cannot compile locally and the annotation costs nothing. The app target itself built clean on both legs -- this was the test bundle only, so the production change was never in question.
…ening it The v34 bump broke DailySdnnMigrationTest's `assertEquals(33, SCHEMA_VERSION)`. I first replaced it with `>= 32`, which was the wrong instinct: it keeps a line that reads as coverage while asserting almost nothing. Removed instead. WhoopDatabaseMigrationChainTest already owns that property and owns it better -- it asserts the chain has no holes AND ends exactly at SCHEMA_VERSION, which is the thing a bump can actually get wrong. Pinning the literal in a test about the v31->v32 migration only made an unrelated file part of every migration diff.
ryanbr
added a commit
that referenced
this pull request
Aug 26, 2026
…1665) The tile only ever showed a deviation, and a deviation with no anchor cannot be read: "+0.9" is a fever or a warm bedroom and nothing says which. The reporter's flu night read "+0.94 dF", which looks like nothing, against 96.4 F on a 24-night mean of 94.4 F. Requested by @justinjor-bit in #1636. The display half; #1663 persisted the value. Scoped to the TILE on both platforms. An earlier draft changed Android's DETAIL and iOS's TILE -- not the same surface -- which left each platform's screen disagreeing with its own drill-in, the #1664 defect reintroduced. The tile is what both platforms share, both can read from skinTempC, and it has no range picker: the detail's range chips key off the displayed series, and a 21-day absolute history would have locked M/3M/6M/1Y permanently, since only the trailing 21 days are ever re-scored. Same rule on both: resolve the displayed night, then lead with its absolute if it has one. Asking about the row already shown is what stops the tile stepping back to an older night when the freshest is import-only. A calibrating night is the reverse and is a real win: recomputeSkinTempDev returns null until the baseline is usable (~4 nights) while the absolute is already measured, so those wearers read "needs ~4 worn nights" today with a real temperature sitting unshown behind it. iOS gains a `secondary` slot kept distinct from `caveat` -- caveat says a reading is unreliable, secondary says what it means. Kotlin gains the twin. Banding, sparkline, range caption and the previous-value delta all follow whichever series is displayed. Verified: 4526 Android tests, 10 Kotlin + 5 Swift for this change, app-build green on both legs with StrandTests executed.
5 tasks done
ryanbr
added a commit
that referenced
this pull request
Sep 3, 2026
…choice (#1845) Reported on #1842: skin temp read as "0.0 dC" -- a deviation against a baseline neither screen names, where a temperature reading should say a temperature. The Health tile already solved this. #1663 stored the nightly ABSOLUTE (dailyMetric.skinTempC) and #1665 made that tile lead with it, because a deviation with no anchor cannot be read: "+0.9" is a fever or a warm bedroom and nothing on a card says which. Three surfaces never came onto the pattern -- Today's Your Cards row, the Key Metrics tile, and the HealthScreen detail (title, unit, chart and every readings row) -- plus both Apple Today layouts. TodayView.dashboardValue was worse still: .hrv, .restingHr and .skinTemp had no carry at all while their neighbours carried. The delta unit is NOT the defect and stays wherever a deviation is genuinely what is shown (#622). What changed is leading with a deviation on a night that measured a real temperature. The carry moved with it. lastSkinTempRow selects on skinTempDevC, so it walked past a CALIBRATING night -- deviation null until the baseline is usable (~4 nights), absolute measured from night one -- leaving those wearers an empty card with a real temperature behind it. lastSkinTempReadingRow takes a row holding either number, and both selectors document why that OR is correct here and is not the #1842 defect: the value IS "whichever of the two this night has", and both fields are read off that ONE row, so an absolute is never paired with another night's deviation. #1846 adds the choice, since the baseline move is the more sensitive illness signal: Settings > Units > Skin temperature, Temperature (default) or vs baseline, on both platforms. A preference and not a filter -- leadReading's prefer argument picks which number is tried first, the other stays the fallback, and the unit always names the scale actually shown rather than the one asked for. Persisted-token parity was caught before shipping: Kotlin wrote Kind.name while Swift wrote rawValue under the same units.skinTempDisplay key. Kotlin's Kind gains a raw matching Swift byte for byte, with a test pinning both tokens. Four re-review findings, all wired-looking code that did nothing: the Health tile ignored the setting on both platforms; six latestVital calls took the new parameter's default and none passed it; that row's predicate still walked past a calibrating night; and the backup whitelist entry was inert because export and import are explicit per key. Also unified the two Kotlin Today chains, which disagreed while the iOS twin's comment claimed they could not -- Your Cards read carriedDay-first, hiding a temperature measured today that todaysOwnReadingWinsOverEitherCarry has asserted against all along. Kotlin full suite 5113/0 with 19 new cases; app-build green on both legs; i18n and doc-comment gates clean. One new Android string across 6 locales, one Apple a11y label spliced across 9 (58 insertions, 0 deletions). Note for testing: skinTempC landed 2026-08-27 and refills on the next scoring pass from raw samples still on disk (analyzeRecent re-scores a 21-night window), so on an install whose newest reading predates that date this is correctly invisible until a rescore.
ryanbr
added a commit
that referenced
this pull request
Sep 3, 2026
Setting Skin temperature to Temperature and getting the same "0.0 dC" back is indistinguishable from a broken toggle. It is not broken: leadReading falls back rather than blanking, so a night that only ever recorded a deviation shows its deviation. Right behaviour, completely invisible. The detail screen now says so under the reading -- "No measured temperature for these nights, showing the difference from your baseline. A sync re-scores recent nights and fills it in." -- and a second sentence covers the case a sync creates: once the 21-night analyzeRecent window refills, the series switches to the absolute column and older deviation-only nights drop out, so the reading count falls with nothing saying why. Why those nights have no temperature, since it is not obvious: the deviation is DERIVED from the absolute, so every night showing a delta had a real temperature at scoring time. It was computed, used, and discarded, because those nights were scored before #1663 added the column on 2026-08-27. A scoring pass re-derives it from raw samples still on disk, which is what the note points at. Three re-review passes found three defects in the notes themselves, all of them copy that could describe the wrong thing -- worse than the silence they were added to fix: The shortened-series note had no leadsAbsolute guard, so it also fired on the deviation-led branch, where the dropped rows are the OPPOSITE kind and the sentence was precisely backwards. Reachable the moment a wearer picks "vs baseline" with one calibrating night in the window. The fallback note fired whenever the NEWEST night lacked an absolute, but its sentence is false if an older night has one. It now requires that none in the window does; that case shows nothing instead. A WHOOP CSV import writes absolute degrees C straight into skinTempDevC (the #622 bimodal field), so an absolute-led series reading skinTempC alone dropped every imported night from a TEMPERATURE chart, then called those real measured temperatures "a baseline difference only". The branch now takes skinTempC ?: skinTempDevC.takeIf(isAbsoluteSkinTemp): mixing is only unsound across scales, and these are the same scale. Both rules are pure and gated inside themselves rather than at the call site, and every case is pinned from the failing direction, so a simplification that drops a guard fails in CI rather than in a screenshot. A test also pins the 20.0 absolute/deviation discriminator directly. Android only, deliberately: Apple's twin is MetricExplorerView, which sources skin temp through the shared Repository.dailyColumn (also feeding Trends and the catalog availability check), so it needs its own branch rather than a change to that mapper. Tracked separately, along with a Fahrenheit import bug found while verifying the degrees F path. Full suite 5124/0, 11 cases in this file. Both notes translated across all 6 locales. Doc-comment and i18n gates clean.
This was referenced Sep 3, 2026
ryanbr
added a commit
that referenced
this pull request
Sep 3, 2026
…aches (#1852) skinTempC is written by the scoring pass, which walks a rolling 21-night window, so nights already outside it when the column landed (2026-08-27, #1663) were never revisited. An install can hold weeks of deviations and a handful of temperatures -- which is what the Temperature setting then has to work with, and on the reporting install left five nights spanning four days against twenty-three deviations spanning three weeks. The raw material is still there. skinTempSample has NO age-based retention; the only DELETE is the #547 plausibility quarantine, which removes rows outside a valid timestamp window rather than old ones. RE-DERIVES, NEVER RECONSTRUCTS. absolute = baseline + deviation is available arithmetic and is wrong: the baseline drifts over weeks, so applying one night's baseline to another night's delta manufactures numbers that read as measurements. This re-runs AnalyticsEngine.skinTempFunnel, the same function over the same night window, so a backfilled night and a scored night come from one code path and cannot disagree. CANNOT DESTROY DATA, which matters because these rows cannot be regenerated: a single-column UPDATE, never an entity upsert that would blank columns it did not carry, and the statement carries AND skinTempC IS NULL so it can only fill a hole. Nothing nulls anything; a night yielding no mean is left alone. Runs after a successful scoring pass and independently of the display preference, so the data is there whichever way the setting is left and switching between Temperature and vs baseline is instant rather than starting work. One page per pass, so a deep history drains over successive passes rather than becoming the #836/#841 battery shape; nights with no skin samples are skipped before their expensive HR read. Three re-review passes each found a real defect in the first cut, and all three are the same mistake at different levels -- reusing a piece of the engine while missing the rule around it: Attribution. The night window is 54 h wide because a night belonging to day D starts on the evening of D-1, so it also contains D-1's night. Handing the funnel everything the window returned averaged TWO nights into one temperature and stored it as D's. The engine attributes by END (#277); sessionsEndingOnDay now does the same, and the reads narrowed to the matched sessions' span as a result. Paging. The candidate query was a plain "oldest N", returning the same nights every pass -- and the oldest are the ones most likely to have lost their raw samples, so the sweep latched on page one and never reached the newer nights that could fill. It now pages on a cursor, a short page signals the sweep saw everything, and the fruitless watermark is recorded only when a WHOLE sweep filled nothing. That watermark keys on the uncapped outstanding count, since the capped one was constant at the page size and could never notice a newly imported night. The 4.0 anchor. It is learned window-wide, and the engine's note says that is safe because the offset cancels in the deviation -- true for a delta, not for an absolute. Re-learning from the old window would put backfilled nights on a different offset from the stored ones and plot two scales as one line. The anchor is passed in from the CURRENT scan window, and a 4.0 without one is declined rather than written on the global default, which maps a second real strap's worn band to 47-72 C. Full suite 5143/0, 16 new cases. Doc-comment and i18n gates clean. Android only; the Apple twin is tracked with its ordering (#1848 first, then #1853).
ryanbr
added a commit
that referenced
this pull request
Sep 3, 2026
Four builds shipped this and none worked on the install it was written for. Each failure was a different piece of wiring, each found only after a person tested it, and none were in the computation: #1852 queried the strap id for rows the engine writes under the computed "-noop" namespace. #1854 ran it inside the scoring pass's new-data gate, so a feature for reprocessing OLD nights only ran when NEW data arrived. #1856 stopped predicting the id altogether, and still did not fill on the reporting install. Every one of those fails to NOTHING rather than to something wrong, which is why each kept reading as "already done" and kept costing another round-trip. The remaining unknown -- whether those nights have banked raw samples at all -- was never settled, so the third fix is unverified rather than known-good. Reverting rather than leaving it merged: on an install where it finds nothing it still walks the tick, and carrying an unverified background job with a standing read cost is worse than not having the feature. The revert is exact: 821 deletions, no insertions, no surviving references, and no schema to unwind -- the backfill only ever added queries and a plain data class, never an entity or a column. Untouched, because none of it depended on the backfill: skin temp leading with the measured temperature, the Settings choice between temperature and baseline, the fallback notes, the per-field HRV / resting-HR carries and the signed-zero fix. skinTempC itself came from #1663 and stays; it is what the display reads. The tracking issue stays open with the design constraints that were genuinely learned: re-derive rather than reconstruct from baseline + deviation, take every device id off the row, attribute sessions by END, page the sweep on a composite cursor, and on a WHOOP 4.0 use the current anchor or decline. Full suite 5127/0 (5149 minus the 22 tests that went with it). Doc-comment and i18n gates clean.
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
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.
Requested by @justinjor-bit in #1636.
What was wrong
The Skin Temperature screen only ever shows a deviation.
+0.2 Δ°Ftoday, and nowhere in the app can you learn what it moved from — a number that could be a fever or a warm bedroom, with no anchor to tell them apart. Their flu night read+0.94 Δ°F, which looks like nothing; the absolute was96.4 °Fagainst a 24-night mean of94.4 °F, on the night their recovery scored 6/100.They were right that the value already exists and is already correct.
wornNightlySkinTempCcomputes a wear-gated nightly mean on every scoring pass, the deviation is taken from it, and the absolute is then discarded. It never reaches disk.Scope: this is the persistence half
Per the one-concern rule, this PR keeps the value; surfacing it is a separate PR. That ordering is deliberate rather than convenient — the column starts filling on the next scoring pass, so by the time any UI reads it users already have populated history instead of an empty column.
No decoding, no new math. The value written is the one the engine already produced.
The change
dailyMetric.skinTempC, nullable and additive.AnalyticsEnginebesideskinTempDevCso the engine's own row is symmetric, and again on the scored row inIntelligenceEngine— at the same line the deviation is written, from the samenightlySkinTempC. The two can never describe different nights, and no second derivation exists to drift.coalesceDay,mergeDaily, the CSV importer fill, and both Swiftwith()helpers. Each of those spells its fields out by name, so a new column is dropped by omission rather than by a compile error — a value that persists correctly and then vanishes on the way to being read, which no migration test would catch. That is what most of the test file guards.Deliberately not a backfill
The issue proposed a migration that recomputes nightly means from
skinTempSample. I did not do that, and I think the simpler answer is also the safer one: NOOP already re-scores from raw on every pass (analyzeRecent(maxDays: 21)), so existing nights refill themselves. History older than the window needs one wider pass, andmaxDaysis already a parameter.A migration-time recompute would be a second implementation of the same mean, free to disagree with the live one, and it would have to reproduce the per-family raw conversion (#938) — which is exactly the hardware-specific surface @justinjor-bit correctly declined to write blind.
On the bimodal column
skinTempDevCis not purely a deviation: CSV/Health imports write an absolute wrist °C into it, andSkinTempDisplayseparates them by magnitude. The new column is unambiguous — always absolute, only ever written by the strap pipeline. Migrating the importers' absolutes into it would be cleaner still, but that is a data migration with real risk and it is not in this PR.Two repo guards fired, and both were right
schema_oracle.json, two byte-identical copies) needed the version, the migration identifier and the column. It caught the bump immediately.analyzeRecentOnCpusits within ~4 KB of the 64 KB JVM method ceiling once JaCoCo instruments it. One more namedcopyargument put it 25 bytes over budget. A named argument is cheap in source and not in bytecode, so the row assembly is extracted toscoredDailyRow; the method now measures 61518 against a 61535 budget, with 4017 bytes of margin.I would not have found either by reading.
Verification
10 Swift tests, 7 Kotlin. Not symmetric, and the gap is the test environment rather than the column: this Android setup has no Robolectric, so Kotlin pins the migration's SQL shape where Swift opens a store and round-trips it (the arrangement
AppleStepHourMigrationTestalready documents). Thewith(...)rebuild helpers are likewise a Swift-only shape — Kotlin's data-classcopycarries every field, so there is no seam there to drop a column at. Every path that exists on both platforms is tested on both.ALTER TABLE, nullable, noDEFAULT, and noDROP/DELETE/UPDATE/INSERT/RENAMEnil, never a fabricated0.0— which would read as a real temperatureDO UPDATElist carries it)Plus: Android 4510 tests green,
compileFullDebugKotlinclean,doc_comment_lintandi18n_audit --ciclean.WhoopStoreandStrandTestsare macOS-only, so those land via CI and an on-demandapp-buildrun — posted below before merge.Not hardware-dependent; no BLE path touched. Nothing reads the new column as a gate, so illness, Charge and the skin-temp marker all still read
skinTempDevCexactly as before.