Repository navigation
Repository.mergeActivityFileSteps: graceful last-wins on duplicate day (parity + no trap) - #588
Merged
Merged
Conversation
…trapping init
mergeActivityFileSteps built its day lookup with
`Dictionary(uniqueKeysWithValues: base.map { ($0.day, $0) })`, which TRAPS (crashes) if `base`
ever carries two rows for the same day. Two reasons to make it graceful:
- Parity: the Kotlin twin (WhoopRepository.mergeActivityFileSteps) already does last-wins
(`for (row in base) byDay[row.day] = row`) — no crash.
- Convention: elsewhere in this same file the day-keyed dictionaries use the safe
`Dictionary(…, uniquingKeysWith: { _, last in last })` form; this one was the outlier.
`mergeDaily` de-dups by day today, so the trap is likely unreachable now — but using a trapping
initializer on merged external data is fragile and diverges from Android. Swap to the two-arg
last-wins init.
Found while investigating #568 (unrelated to the steps-count fix in #584). App-target Swift, so
no local build (GRDB/Combine wall) — the two-arg Dictionary init form was type-checked in
isolation; `mergeActivityFileSteps` is covered by VitalSourceResolutionTests on macOS CI.
DX23876
pushed a commit
to DX23876/noop
that referenced
this pull request
Jul 22, 2026
…trapping init (ryanbr#588) mergeActivityFileSteps built its day lookup with `Dictionary(uniqueKeysWithValues: base.map { ($0.day, $0) })`, which TRAPS (crashes) if `base` ever carries two rows for the same day. Two reasons to make it graceful: - Parity: the Kotlin twin (WhoopRepository.mergeActivityFileSteps) already does last-wins (`for (row in base) byDay[row.day] = row`) — no crash. - Convention: elsewhere in this same file the day-keyed dictionaries use the safe `Dictionary(…, uniquingKeysWith: { _, last in last })` form; this one was the outlier. `mergeDaily` de-dups by day today, so the trap is likely unreachable now — but using a trapping initializer on merged external data is fragile and diverges from Android. Swap to the two-arg last-wins init. Found while investigating ryanbr#568 (unrelated to the steps-count fix in ryanbr#584). App-target Swift, so no local build (GRDB/Combine wall) — the two-arg Dictionary init form was type-checked in isolation; `mergeActivityFileSteps` is covered by VitalSourceResolutionTests on macOS CI.
ryanbr
added a commit
that referenced
this pull request
Jul 27, 2026
…ttery policies every second (#886) From a user report of NOOP at 51.75% of battery over 6h31m, confirmed by a second user. Two safe changes, plus the release-hygiene fix that was blocking every Android PR from going green. 1. LOG HOW LONG A LINK WAS HELD. Reaching STATE_CONNECTED zeroes failedReconnectAttempts, and that counter is the ONLY input to scanModeForReconnectAttempts. So a strap that connects and drops repeatedly — edge of range, phone in another room — never accumulates a streak, and every rescan stays on SCAN_MODE_LOW_LATENCY, which is exactly what the PR #588 backoff exists to escape. The #982 guard covers the never-bonded variant of that loop; a bonded-but-short-lived link is not covered. That is a hypothesis, not a measurement, so this changes no behaviour — it makes the pattern readable in an ordinary strap log: Disconnected (status=19); rescanning in 3s (attempt 1, held 24s) A run of drops that all say `attempt 1` with a short `held` IS the bug, with no instrumentation. Attempts climbing normally rules it out and points elsewhere. Snapshotted and cleared at the top of handleDisconnect — a stale linkUpSinceMs surviving into a drop that never reached STATE_CONNECTED would report a hold time for a link that was never up, the diagnostic inventing the evidence it exists to find. handleDisconnect is the single teardown funnel (its own comment: it "publishes connected=false and closes + nulls gatt"), so the clear cannot leak. 2. STOP RE-ASKING THE BATTERY POLICIES EVERY SECOND. The foreground-service collector ticks at live-HR cadence (~1/s while connected) and ran onBatteryUpdate AND onCriticalBattery on every tick. Both early-exit on their own persisted once-per-crossing gates — but not before each does a SharedPreferences read, an areNotificationsEnabled() binder call and an ensureChannel(). Roughly two binder calls and half a dozen prefs reads every second, to re-answer a question the strap only changes every ~8 minutes. Now gated on (SoC, charging) changing, the same shape as the lastRuntimeEvalPct gate two lines below. Gated on the PAIR, not the percentage: onBatteryUpdate owns the charge-complete and re-arm transitions, which key off `charging`. One behavioural delta, stated rather than papered over: enabling the battery-alerts pref while already past a threshold now waits for the next (pct, charging) change instead of firing within a second — up to one battery-report cycle. 3. LOCALIZE THE 9.2.0 WHAT'S NEW TITLE. The 9.2.0 release left main FAILING i18n-coverage. appchangelog-gen writes the title as a raw Kotlin literal; extraction into a string resource is a separate step that normally happens in the PR flow where the gate catches it, and fork-release.yml commits straight to main. That also means the 9.2.0 title shipped UNTRANSLATED on Android — the only one of 240+ release entries to do so. Resource added under the existing convention and translated in all five locale files. The convention was DERIVED, not guessed: first six slug words plus sha1(unescaped value)[:8], which reproduces 267 of 268 existing keys — the single exception being the key I had first written by eyeballing a few examples and assuming a ~40-character truncation. values-zh closed rather than its allowance raised, because Tools/i18n_extra_locale_baseline.txt says in its own header to ratchet DOWN, never up. This will recur on every release until the extraction runs inside fork-release.yml. RULED OUT while looking, all already defended: the FGS notification dedupes on a connected|backfilling|recovery|battery key (live HR is not in it); the Glance widget push has a cheap non-suspending PushGate; the offload chunk counter republishes every ~10 chunks; connection priority does not escalate for live HR by default; reconnect backoff is capped 3s->60s; IllnessWatch.evaluate is bounded to <=31 days of means; uiString is ~1us per call. NOT DONE, deliberately: making the reconnect streak survive a short-lived connection. That reset exists to stop a flapping co-resident band from freezing the link (#173 regression, where a climbing counter escalated to PASSIVE autoConnect and stalled). Changing it blind, on an unverified hypothesis, on the path that keeps the strap connected at all, trades a battery complaint for a strap that does not come back. The hold-time log is what tells us whether it is worth doing. Register verified rather than assumed: all three Android locales are decisively informal (es 35:1, de 118:6, pt 241:0 on unambiguous markers), matching the strings added here. No CI job compiles Kotlin, so every symbol was checked against its declaration by hand.
simoncad7
pushed a commit
to simoncad7/noop
that referenced
this pull request
Aug 17, 2026
…trapping init (ryanbr#588) mergeActivityFileSteps built its day lookup with `Dictionary(uniqueKeysWithValues: base.map { ($0.day, $0) })`, which TRAPS (crashes) if `base` ever carries two rows for the same day. Two reasons to make it graceful: - Parity: the Kotlin twin (WhoopRepository.mergeActivityFileSteps) already does last-wins (`for (row in base) byDay[row.day] = row`) — no crash. - Convention: elsewhere in this same file the day-keyed dictionaries use the safe `Dictionary(…, uniquingKeysWith: { _, last in last })` form; this one was the outlier. `mergeDaily` de-dups by day today, so the trap is likely unreachable now — but using a trapping initializer on merged external data is fragile and diverges from Android. Swap to the two-arg last-wins init. Found while investigating ryanbr#568 (unrelated to the steps-count fix in ryanbr#584). App-target Swift, so no local build (GRDB/Combine wall) — the two-arg Dictionary init form was type-checked in isolation; `mergeActivityFileSteps` is covered by VitalSourceResolutionTests on macOS CI.
simoncad7
pushed a commit
to simoncad7/noop
that referenced
this pull request
Aug 17, 2026
…ttery policies every second (ryanbr#886) From a user report of NOOP at 51.75% of battery over 6h31m, confirmed by a second user. Two safe changes, plus the release-hygiene fix that was blocking every Android PR from going green. 1. LOG HOW LONG A LINK WAS HELD. Reaching STATE_CONNECTED zeroes failedReconnectAttempts, and that counter is the ONLY input to scanModeForReconnectAttempts. So a strap that connects and drops repeatedly — edge of range, phone in another room — never accumulates a streak, and every rescan stays on SCAN_MODE_LOW_LATENCY, which is exactly what the PR ryanbr#588 backoff exists to escape. The ryanbr#982 guard covers the never-bonded variant of that loop; a bonded-but-short-lived link is not covered. That is a hypothesis, not a measurement, so this changes no behaviour — it makes the pattern readable in an ordinary strap log: Disconnected (status=19); rescanning in 3s (attempt 1, held 24s) A run of drops that all say `attempt 1` with a short `held` IS the bug, with no instrumentation. Attempts climbing normally rules it out and points elsewhere. Snapshotted and cleared at the top of handleDisconnect — a stale linkUpSinceMs surviving into a drop that never reached STATE_CONNECTED would report a hold time for a link that was never up, the diagnostic inventing the evidence it exists to find. handleDisconnect is the single teardown funnel (its own comment: it "publishes connected=false and closes + nulls gatt"), so the clear cannot leak. 2. STOP RE-ASKING THE BATTERY POLICIES EVERY SECOND. The foreground-service collector ticks at live-HR cadence (~1/s while connected) and ran onBatteryUpdate AND onCriticalBattery on every tick. Both early-exit on their own persisted once-per-crossing gates — but not before each does a SharedPreferences read, an areNotificationsEnabled() binder call and an ensureChannel(). Roughly two binder calls and half a dozen prefs reads every second, to re-answer a question the strap only changes every ~8 minutes. Now gated on (SoC, charging) changing, the same shape as the lastRuntimeEvalPct gate two lines below. Gated on the PAIR, not the percentage: onBatteryUpdate owns the charge-complete and re-arm transitions, which key off `charging`. One behavioural delta, stated rather than papered over: enabling the battery-alerts pref while already past a threshold now waits for the next (pct, charging) change instead of firing within a second — up to one battery-report cycle. 3. LOCALIZE THE 9.2.0 WHAT'S NEW TITLE. The 9.2.0 release left main FAILING i18n-coverage. appchangelog-gen writes the title as a raw Kotlin literal; extraction into a string resource is a separate step that normally happens in the PR flow where the gate catches it, and fork-release.yml commits straight to main. That also means the 9.2.0 title shipped UNTRANSLATED on Android — the only one of 240+ release entries to do so. Resource added under the existing convention and translated in all five locale files. The convention was DERIVED, not guessed: first six slug words plus sha1(unescaped value)[:8], which reproduces 267 of 268 existing keys — the single exception being the key I had first written by eyeballing a few examples and assuming a ~40-character truncation. values-zh closed rather than its allowance raised, because Tools/i18n_extra_locale_baseline.txt says in its own header to ratchet DOWN, never up. This will recur on every release until the extraction runs inside fork-release.yml. RULED OUT while looking, all already defended: the FGS notification dedupes on a connected|backfilling|recovery|battery key (live HR is not in it); the Glance widget push has a cheap non-suspending PushGate; the offload chunk counter republishes every ~10 chunks; connection priority does not escalate for live HR by default; reconnect backoff is capped 3s->60s; IllnessWatch.evaluate is bounded to <=31 days of means; uiString is ~1us per call. NOT DONE, deliberately: making the reconnect streak survive a short-lived connection. That reset exists to stop a flapping co-resident band from freezing the link (ryanbr#173 regression, where a climbing counter escalated to PASSIVE autoConnect and stalled). Changing it blind, on an unverified hypothesis, on the path that keeps the strap connected at all, trades a battery complaint for a strap that does not come back. The hold-time log is what tells us whether it is worth doing. Register verified rather than assumed: all three Android locales are decisively informal (es 35:1, de 118:6, pt 241:0 on unambiguous markers), matching the strings added here. No CI job compiles Kotlin, so every symbol was checked against its declaration by hand.
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.
A one-line robustness + parity nit, found on a parity pass while investigating #568 (independent of the steps-count fix #584 and the still-open #585).
The nit
Repository.mergeActivityFileSteps(Strand/Data/Repository.swift) built its day lookup with:uniqueKeysWithValuestraps (crashes) ifbaseever contains two rows for the sameday. Two things make this worth swapping:WhoopRepository.mergeActivityFileStepsalready does graceful last-wins (for (row in base) byDay[row.day] = row), so the two platforms diverge on duplicate-day input (iOS crashes, Android doesn't).Repository.swiftuse the safeDictionary(…, uniquingKeysWith: { _, last in last }). This call was the odd one out.Severity
Likely latent, not live —
mergeDaily(which producesbase) de-dups by day today, so the trap is probably unreachable right now. But a trapping initializer on merged external data is fragile: it becomes a crash the day an upstream query changes. Matching Android's (and the file's own) graceful form removes that footgun for free.Fix
Last-wins, matching Android and the rest of the file. No behaviour change except that a duplicate day no longer crashes.
Verification
App-target Swift (
Strand/Data/Repository.swift), so no local build (GRDB/Combine wall on Linux). The two-argDictionaryinit form was type-checked in isolation (compiles), andmergeActivityFileStepsis exercised byVitalSourceResolutionTests(testActivityFileStepsFillMissingStepDaysOnly) on macOS. Needs anxcodebuild -scheme Strand test/app-build.ymlrun to fully validate, like the other recent app-target changes.