Fix Health Connect sleep session fragmentation - #196
Conversation
|
Warning Review limit reached
Next review available in: 18 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis change adds typed Android Health Connect sleep-session and heart-rate exports. It normalizes payloads, replaces existing records, coordinates retries and concurrent synchronization, propagates failed writes, and adds Dart and Android regression coverage. ChangesHealth Connect export
Estimated code review effort: 5 (Critical) | ~90 minutes Possibly related PRs
Suggested labels: Sequence Diagram(s)sequenceDiagram
participant AppState
participant HealthExporter
participant SleepSessionExporter
participant HeartRateExporter
participant HealthConnect
AppState->>HealthExporter: Start guarded export
HealthExporter->>SleepSessionExporter: Export newest Android sleep session
SleepSessionExporter->>HealthConnect: Replace consolidated sleep record
HealthConnect-->>SleepSessionExporter: Return result
HealthExporter->>HeartRateExporter: Export normalized heart-rate day
HeartRateExporter->>HealthConnect: Replace batched heart-rate record
HealthConnect-->>HeartRateExporter: Return result
HealthExporter-->>AppState: Return aggregate success and retry state
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🟡 Changes recommended
The new sleep-session replace path can currently write an empty-stage sleep session and the native Health Connect work is launched on the main dispatcher, both of which can cause incorrect exports and/or UI jank.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Fixes Android Health Connect sleep export fragmentation by replacing per-stage generic writeHealthData() writes with a single native SleepSessionRecord containing all stages, while keeping Apple Health behavior intact and making retries/manual sync more robust.
Changes:
- Added a project-local Android
MethodChannelwriter to delete+replace one sleep session record with its full stage list. - Normalized sleep stages (order, clip, de-overlap, drop zero-duration) and used idempotent replace semantics for re-exports.
- Propagated
falseresults from delete/write APIs and enabled manual sync to bypass retry backoff/caps.
File summaries
| File | Description |
|---|---|
| test/health_sleep_export_test.dart | Adds regression coverage for normalization, single-call stage export, idempotent replace, retry forcing, and concurrency queuing. |
| lib/state/app_state.dart | Forces retry when user triggers manual health sync. |
| lib/health/health_sleep_session.dart | Adds sleep-session normalization + method-channel writer/exporter wrappers. |
| lib/health/health_export.dart | Switches Android sleep export to native replace, improves idempotency/retry behavior by honoring false results. |
| android/app/src/main/kotlin/wtf/openstrap/openstrap_edge/NativeChannels.kt | Registers the new native sleep writer channel. |
| android/app/src/main/kotlin/wtf/openstrap/openstrap_edge/HealthConnectSleepWriter.kt | Implements the native Health Connect delete+insert of a single sleep session with stages. |
| android/app/build.gradle.kts | Adds Health Connect client dependency needed by the native writer. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 3
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| Future<bool> replace(Map<String, dynamic> bundle) async { | ||
| final session = normalizeHealthSleepSession(bundle); | ||
| if (session == null) return true; | ||
| return writer.replace(session); | ||
| } |
There was a problem hiding this comment.
Fixed in 044d507. A detected sleep window whose stages normalize to an empty list now returns false without invoking the native writer. This keeps the day retryable and cannot delete an existing valid session.
| val rawStages = call.argument<List<Map<String, Any?>>>("stages").orEmpty() | ||
| val stages = rawStages.mapNotNull(::buildStage).sortedBy { it.startTime } | ||
| var previousEnd = start | ||
| for (stage in stages) { | ||
| if (stage.startTime.isBefore(start) || stage.endTime.isAfter(end)) return null | ||
| if (!stage.startTime.isBefore(stage.endTime)) return null | ||
| if (stage.startTime.isBefore(previousEnd)) return null | ||
| previousEnd = stage.endTime | ||
| } |
There was a problem hiding this comment.
Fixed in 044d507. The native parser now rejects an empty or fully invalid stage list before any Health Connect delete or insert is attempted.
| private const val RECORDING_METHOD_AUTOMATIC = 2 | ||
| private val scope = CoroutineScope(SupervisorJob() + Dispatchers.Main.immediate) | ||
| private val replaceMutex = Mutex() | ||
|
|
There was a problem hiding this comment.
Fixed in 044d507. Health Connect parsing/delete/insert work now runs inside withContext(Dispatchers.IO), while the MethodChannel result is delivered after returning to the main scope.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/src/main/kotlin/wtf/openstrap/openstrap_edge/HealthConnectSleepWriter.kt`:
- Around line 74-75: Update the start and end timestamp parsing in the
sleep-writing method to use the same defensive Number conversion as buildStage:
read each argument without an unchecked Long cast, safely cast it to Number,
convert it with toLong(), and return null when absent or invalid. Keep
Instant.ofEpochMilli as the final conversion.
- Around line 66-69: Update the broad catch in the sleep-session replacement
flow to rethrow kotlinx.coroutines.CancellationException before handling other
Exception values, preserving coroutine cancellation while still returning false
and logging unexpected failures. Add the explicit detekt suppression documenting
why the broad catch is required.
- Line 26: The SleepSessionRecord metadata setup must match connect-client
1.1.0-alpha07. Replace the local RECORDING_METHOD_AUTOMATIC constant and direct
Metadata construction with Metadata.autoRecorded(...) using
Metadata.RECORDING_METHOD_AUTOMATICALLY_RECORDED when available, and
verify/update the SleepSessionRecord constructor named parameters against the
pinned API, especially title and stages.
In `@lib/health/health_export.dart`:
- Line 621: Update the write predicate in the sleep-stage export branch to use
the same isApple predicate passed to healthDeleteTypes, replacing the broader
!Platform.isAndroid check. Preserve the existing sleep-stage write behavior
while ensuring deletion and writing agree on non-Apple platforms.
In `@lib/health/health_sleep_session.dart`:
- Around line 106-121: Consolidate hypnogram label decoding in _stageOf, and
remove the duplicate label switch from _sleepType in health_export.dart. Update
the health_export.dart flow to call _stageOf and then map the resulting
HealthSleepStage to the appropriate HealthDataType, preserving the existing
Apple and Android export behavior while keeping one shared vocabulary source.
In `@lib/state/app_state.dart`:
- Around line 389-392: Guard healthSyncNow with a single-flight state field,
matching the existing _kickSyncBurst and _orphanRecovery pattern in the
surrounding class. Return the in-flight Future or otherwise prevent a second
forced export while the first _healthExport.exportAll(forceRetry: true) is
running, and clear the guard when it completes so later syncs can proceed.
In `@test/health_sleep_export_test.dart`:
- Around line 228-246: Update the test around
HealthConnectSleepSessionExporter.replace to track the method-channel handler
invocation count independently of storedParents, then assert the expected call
count after both replacements. Keep the storedParents assertions focused on the
final payload, and do not use its length to verify replacement behavior.
- Around line 286-288: In the concurrency test around the native replace calls,
replace the single Future<void>.delayed(Duration.zero) yield with await
pumpEventQueue() so all scheduled tasks are drained before asserting calls
remains 1. Add the flutter_test import at the top of the file if it is not
already present.
🪄 Autofix
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 478dfd04-e981-4255-ac60-8c9851d48bf3
📒 Files selected for processing (7)
android/app/build.gradle.ktsandroid/app/src/main/kotlin/wtf/openstrap/openstrap_edge/HealthConnectSleepWriter.ktandroid/app/src/main/kotlin/wtf/openstrap/openstrap_edge/NativeChannels.ktlib/health/health_export.dartlib/health/health_sleep_session.dartlib/state/app_state.darttest/health_sleep_export_test.dart
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/state/app_state.dart`:
- Around line 391-393: Route the automatic export in `_afterDrain` through
`_healthExportSingleFlight.run(...)`, matching the existing `healthSyncNow()`
guard so every `_healthExport.exportAll()` call is serialized. Add a regression
test that overlaps an automatic export with a forced manual export and verifies
both complete without conflicting record or retry-cursor updates.
🪄 Autofix
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c598dd7f-4227-47e3-988a-e1f691038504
📒 Files selected for processing (5)
android/app/src/main/kotlin/wtf/openstrap/openstrap_edge/HealthConnectSleepWriter.ktlib/health/health_export.dartlib/health/health_sleep_session.dartlib/state/app_state.darttest/health_sleep_export_test.dart
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/src/main/kotlin/wtf/openstrap/openstrap_edge/HealthConnectSleepWriter.kt`:
- Around line 27-39: Update sleepCleanupRange so its returned cleanupEnd cannot
extend beyond the exported day interval used by _exportDay. Clamp the calculated
end to the day’s end boundary, or skip/handle cross-boundary sleep windows
separately, while preserving valid cleanup for windows entirely within the day.
In
`@android/app/src/test/kotlin/wtf/openstrap/openstrap_edge/HealthConnectSleepWriterTest.kt`:
- Around line 17-41: Add a test covering the early-start branch in
sleepCleanupRange, using a session start before local noon on the previous day
so the function returns the session start rather than calculatedStart. Assert
the cleanup range preserves that early start and still uses the expected session
end boundary.
In `@lib/health/health_export.dart`:
- Around line 416-419: Update the !shouldAttempt branch in the export flow so
the under-cap backoff case also calls exportBulk(null) instead of returning 0.
Preserve the existing attempts >= _kMaxExportAttempts fallback and ensure
priority-day backoff still proceeds with bulk export while skipping the sleep
retry.
- Around line 65-71: Align the heart-rate delete filter with the native
batch-writer selection used by exportContinuousHeartRateDay: use the same
Android/platform predicate at the heart-rate delete path instead of
isApplePlatform. Ensure heart-rate deletion occurs whenever the generic
per-sample write fallback is not used, preventing repeated exports from
duplicating samples.
In `@lib/health/health_heart_rate_batch.dart`:
- Around line 105-111: Update the sample export loop around writeGeneric to
clamp each sample’s computed end time to the export end boundary, while
preserving the existing one-minute interval for earlier samples. Add a
regression test covering a sample at end minus 30 seconds and verify the written
interval does not extend beyond end.
In `@lib/state/app_state.dart`:
- Around line 391-396: Update _runHealthExport and its single-flight
coordination so a forceRetry: true caller joining an in-flight non-forced export
chains a subsequent forced _healthExport.exportAll run and returns that forced
result; preserve single-flight behavior for matching requests. Add a regression
test covering an in-flight non-forced export followed by healthSyncNow(),
asserting exportAll is invoked again with forceRetry enabled.
🪄 Autofix
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8c9233ea-6feb-40a5-bfcd-5d4732cddbc1
📒 Files selected for processing (15)
.gitignoreandroid/app/build.gradle.ktsandroid/app/src/main/kotlin/wtf/openstrap/openstrap_edge/HealthConnectHeartRateWriter.ktandroid/app/src/main/kotlin/wtf/openstrap/openstrap_edge/HealthConnectSleepWriter.ktandroid/app/src/main/kotlin/wtf/openstrap/openstrap_edge/NativeChannels.ktandroid/app/src/test/kotlin/wtf/openstrap/openstrap_edge/HealthConnectSleepWriterTest.ktdocs/superpowers/plans/2026-08-06-health-connect-legacy-sleep-cleanup.mddocs/superpowers/plans/2026-08-06-health-connect-priority-and-heart-rate-batch.mddocs/superpowers/specs/2026-08-06-health-connect-heart-rate-batch-design.mddocs/superpowers/specs/2026-08-06-health-connect-legacy-sleep-cleanup-design.mdlib/health/health_export.dartlib/health/health_heart_rate_batch.dartlib/state/app_state.darttest/health_heart_rate_export_test.darttest/health_sleep_export_test.dart
| internal fun sleepCleanupRange(start: Instant, end: Instant, zoneId: ZoneId): SleepCleanupRange { | ||
| require(start.isBefore(end)) | ||
| val localEnd = end.atZone(zoneId) | ||
| val endDate = if (localEnd.toLocalTime().isBefore(LocalTime.NOON)) { | ||
| localEnd.toLocalDate() | ||
| } else { | ||
| localEnd.toLocalDate().plusDays(1) | ||
| } | ||
| val cleanupEnd = endDate.atTime(LocalTime.NOON).atZone(zoneId).toInstant() | ||
| val calculatedStart = endDate.minusDays(1).atTime(LocalTime.NOON).atZone(zoneId).toInstant() | ||
| val cleanupStart = if (start.isBefore(calculatedStart)) start else calculatedStart | ||
| return SleepCleanupRange(cleanupStart, cleanupEnd) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the sleep-window derivation to see whether an offset can land at/after local noon.
set -euo pipefail
fd -e dart . lib --exec rg -n -C4 'offset_ms|onset_ms' {} \; | head -100
printf '\n--- sleep window segmentation source ---\n'
fd -i 'sleep' -e dart . libRepository: OpenStrap/edge
Length of output: 4333
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- candidate files ---\n'
fd -e dart . lib | sort
fd -e kt android . | sort
printf '\n--- HealthConnectSleepWriter outline ---\n'
ast-grep outline android/app/src/main/kotlin/wtf/openstrap/openstrap_edge/HealthConnectSleepWriter.kt --view expanded || true
printf '\n--- HealthConnectSleepWriter relevant lines ---\n'
cat -n android/app/src/main/kotlin/wtf/openstrap/openstrap_edge/HealthConnectSleepWriter.kt | sed -n '1,140p'
printf '\n--- references to sleepCleanupRange and replace ---\n'
rg -n "sleepCleanupRange|fun replace|class.*Sleep|replace\\(" android/lib -SRepository: OpenStrap/edge
Length of output: 243
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- candidate files ---'
fd -e dart . lib | sort
fd -e kt android . | sort
printf '%s\n' ''
printf '%s\n' '--- HealthConnectSleepWriter outline ---'
ast-grep outline android/app/src/main/kotlin/wtf/openstrap/openstrap_edge/HealthConnectSleepWriter.kt --view expanded || true
printf '%s\n' ''
printf '%s\n' '--- HealthConnectSleepWriter relevant lines ---'
cat -n android/app/src/main/kotlin/wtf/openstrap/openstrap_edge/HealthConnectSleepWriter.kt | sed -n '1,140p'
printf '%s\n' ''
printf '%s\n' '--- references to sleepCleanupRange and replace ---'
rg -n "sleepCleanupRange|fun replace|class.*Sleep|replace\\(" android/lib -S || trueRepository: OpenStrap/edge
Length of output: 13949
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- sleep-related Dart files ---'
fd -e dart . lib | rg -i 'sleep|heal' || true
printf '%s\n' ''
printf '%s\n' '--- sleep session normalizer relevant lines ---'
cat -n lib/health/health_sleep_session.dart | sed -n '1,240p'
printf '%s\n' ''
printf '%s\n' '--- derivation files sleep terms ---'
rg -n -C3 "sleep(\\.window|Window|window)|sleep_|onset|offset|wake_ts|startTime|endTime|offset_ms|onset_ms" lib lib/test --glob '*.dart' || true
printf '%s\n' ''
printf '%s\n' '--- day_id groupby and bulk export references ---'
rg -n -C4 "day_id|groupBy|exportAll|replaceSleepSession|sleepCleanupRange|sleep.window" lib --glob '*.dart' || trueRepository: OpenStrap/edge
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- exact search for replaceSleepSession call sites ---'
rg -n "replaceSleepSession|HealthConnectSleepWriter|sleepCleanupRange" lib android --glob '*.dart' --glob '*.kt' || true
printf '%s\n' ''
printf '%s\n' '--- sleep export/write channel usage ---'
rg -n "openstrap/health_connect_sleep|HealthConnect|sleep_session|startTime|endTime" lib --glob '*.dart' --max-count 120 || true
printf '%s\n' ''
printf '%s\n' '--- targetDayWindow and day attribution region ---'
cat -n lib/compute/derivation_engine.dart | sed -n '4300,4345p'
cat -n lib/compute/substrate.dart | sed -n '428,434p'
printf '%s\n' ''
python3 - <<'PY'
from datetime import datetime, timezone, timedelta
def cleanup_range_ms(epoch_start_ms, epoch_end_ms, tz_offset_ms):
zone = timezone(timedelta(milliseconds=tz_offset_ms))
start = datetime.fromtimestamp(epoch_start_ms/1000, tz=zone)
end = datetime.fromtimestamp(epoch_end_ms/1000, tz=zone)
local_end = end.astimezone(zone)
if local_end.time() < datetime(2000,1,1,12,0).time():
end_date = local_end.date()
else:
end_date = local_end.date() + timedelta(days=1)
cleanup_end = datetime.combine(end_date, datetime.min.time()).replace(hour=12, tzinfo=zone).timestamp()*1000
calculated_start_ms = (datetime.combine(end_date - timedelta(days=1), datetime.min.time()).replace(hour=12, tzinfo=zone).timestamp()*1000)
cleanup_start_ms = min(epoch_start_ms, calculated_start_ms)
return cleanup_start_ms, cleanup_end_ms
def to_window(epoch):
end = datetime.fromtimestamp(epoch/1000, tz=timezone.utc) + timedelta(hours=5)
return end.date().isoformat(), end.time().isoformat()
base = datetime(2026, 8, 3, 0, 0, tzinfo=timezone.utc).timestamp()*1000
cases = [
("nap ending 13:00 day N and following night ending 07:00 day N+1",
base + 13*3600*1000, base + 24*3600*1000 + 7*3600*1000, +5*3600*1000),
]
for label, start, end, offset in cases:
cstart, cend = cleanup_range_ms(start, end, offset)
print(label)
print("session:", to_window(start), to_window(end))
print("cleanup:", to_window(cstart), to_window(cend))
print("overlap:", (max(start, cstart) < min(end, cend)) and (max(start, cstart) < min(cend, end)))
PY
printf '%s\n' ''
printf '%s\n' '--- tests around sleepCleanupRange if present ---'
fd -e kt . | rg -i test || true
rg -n "sleepCleanupRange|SleepCleanupRange|noon|LocalTime.NOON|cleanup" . --glob '*.kt' --glob '*.test.*' --glob '*_test.*' || trueRepository: OpenStrap/edge
Length of output: 10473
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- HealthConnect sleep writer call path ---'
cat -n lib/health/health_sleep_session.dart | sed -n '120,210p'
cat -n lib/health/health_export.dart | sed -n '560,620p'
cat -n lib/health/health_export.dart | sed -n '780,850p'
printf '%s\n' ''
printf '%s\n' '--- day bundle grouping and replacement context ---'
cat -n lib/health/health_export.dart | sed -n '540,560p'
rg -n -C2 "day_id|sleep_session_candidates|fromMap|toMap|sleep:" lib/health/health_sleep_session.dart lib/health/health_export.dart
printf '%s\n' ''
printf '%s\n' '--- derive window and daylight search horizon ---'
cat -n lib/compute/derivation_engine.dart | sed -n '540,580p'
cat -n lib/compute/substrate.dart | sed -n '464,488p'
cat -n lib/compute/derivation_engine.dart | sed -n '1932,1950p'Repository: OpenStrap/edge
Length of output: 15423
Keep sleep cleanup inside the day window.
_exportDay marks success if the sleep replace succeeds, then deletes other data only for [dayStart, dayEnd). If a sleep window exits the local day, replace() can then delete records beyond dayEnd while the day is considered exported, and later data for that exported day is not re-published in that pass. Clamp cleanupRange.end to the exported day interval, or avoid exporting a sleep window that crosses the day boundary without handling the cleanup boundary separately.
🤖 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
`@android/app/src/main/kotlin/wtf/openstrap/openstrap_edge/HealthConnectSleepWriter.kt`
around lines 27 - 39, Update sleepCleanupRange so its returned cleanupEnd cannot
extend beyond the exported day interval used by _exportDay. Clamp the calculated
end to the day’s end boundary, or skip/handle cross-boundary sleep windows
separately, while preserving valid cleanup for windows entirely within the day.
| @Test | ||
| fun cleanupRangeIncludesLegacyFragmentThatStartsBeforeRecomputedSession() { | ||
| val sessionStart = localInstant(2026, 8, 6, 1, 36) | ||
| val sessionEnd = localInstant(2026, 8, 6, 8, 20) | ||
| val staleFragmentStart = localInstant(2026, 8, 5, 23, 34) | ||
|
|
||
| val range = sleepCleanupRange(sessionStart, sessionEnd, berlin) | ||
|
|
||
| assertEquals(localInstant(2026, 8, 5, 12, 0), range.start) | ||
| assertEquals(localInstant(2026, 8, 6, 12, 0), range.end) | ||
| assertTrue(!staleFragmentStart.isBefore(range.start)) | ||
| assertTrue(staleFragmentStart.isBefore(range.end)) | ||
| } | ||
|
|
||
| @Test | ||
| fun cleanupRangeUsesLocalNoonAcrossDstTransition() { | ||
| val sessionStart = localInstant(2026, 10, 25, 1, 30) | ||
| val sessionEnd = localInstant(2026, 10, 25, 9, 0) | ||
|
|
||
| val range = sleepCleanupRange(sessionStart, sessionEnd, berlin) | ||
|
|
||
| assertEquals(localInstant(2026, 10, 24, 12, 0), range.start) | ||
| assertEquals(localInstant(2026, 10, 25, 12, 0), range.end) | ||
| assertEquals(25, Duration.between(range.start, range.end).toHours()) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a test for the early-start branch of sleepCleanupRange.
Both tests exercise only the path where calculatedStart wins on line 37. The branch that returns the session start (a session starting before local noon of the previous day) has no coverage. That branch protects a long session from a truncated cleanup window.
♻️ Proposed test
+ `@Test`
+ fun cleanupRangeExtendsBackToAnEarlySessionStart() {
+ val sessionStart = localInstant(2026, 8, 5, 9, 15)
+ val sessionEnd = localInstant(2026, 8, 6, 8, 20)
+
+ val range = sleepCleanupRange(sessionStart, sessionEnd, berlin)
+
+ assertEquals(sessionStart, range.start)
+ assertEquals(localInstant(2026, 8, 6, 12, 0), range.end)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Test | |
| fun cleanupRangeIncludesLegacyFragmentThatStartsBeforeRecomputedSession() { | |
| val sessionStart = localInstant(2026, 8, 6, 1, 36) | |
| val sessionEnd = localInstant(2026, 8, 6, 8, 20) | |
| val staleFragmentStart = localInstant(2026, 8, 5, 23, 34) | |
| val range = sleepCleanupRange(sessionStart, sessionEnd, berlin) | |
| assertEquals(localInstant(2026, 8, 5, 12, 0), range.start) | |
| assertEquals(localInstant(2026, 8, 6, 12, 0), range.end) | |
| assertTrue(!staleFragmentStart.isBefore(range.start)) | |
| assertTrue(staleFragmentStart.isBefore(range.end)) | |
| } | |
| @Test | |
| fun cleanupRangeUsesLocalNoonAcrossDstTransition() { | |
| val sessionStart = localInstant(2026, 10, 25, 1, 30) | |
| val sessionEnd = localInstant(2026, 10, 25, 9, 0) | |
| val range = sleepCleanupRange(sessionStart, sessionEnd, berlin) | |
| assertEquals(localInstant(2026, 10, 24, 12, 0), range.start) | |
| assertEquals(localInstant(2026, 10, 25, 12, 0), range.end) | |
| assertEquals(25, Duration.between(range.start, range.end).toHours()) | |
| } | |
| `@Test` | |
| fun cleanupRangeIncludesLegacyFragmentThatStartsBeforeRecomputedSession() { | |
| val sessionStart = localInstant(2026, 8, 6, 1, 36) | |
| val sessionEnd = localInstant(2026, 8, 6, 8, 20) | |
| val staleFragmentStart = localInstant(2026, 8, 5, 23, 34) | |
| val range = sleepCleanupRange(sessionStart, sessionEnd, berlin) | |
| assertEquals(localInstant(2026, 8, 5, 12, 0), range.start) | |
| assertEquals(localInstant(2026, 8, 6, 12, 0), range.end) | |
| assertTrue(!staleFragmentStart.isBefore(range.start)) | |
| assertTrue(staleFragmentStart.isBefore(range.end)) | |
| } | |
| `@Test` | |
| fun cleanupRangeUsesLocalNoonAcrossDstTransition() { | |
| val sessionStart = localInstant(2026, 10, 25, 1, 30) | |
| val sessionEnd = localInstant(2026, 10, 25, 9, 0) | |
| val range = sleepCleanupRange(sessionStart, sessionEnd, berlin) | |
| assertEquals(localInstant(2026, 10, 24, 12, 0), range.start) | |
| assertEquals(localInstant(2026, 10, 25, 12, 0), range.end) | |
| assertEquals(25, Duration.between(range.start, range.end).toHours()) | |
| } | |
| `@Test` | |
| fun cleanupRangeExtendsBackToAnEarlySessionStart() { | |
| val sessionStart = localInstant(2026, 8, 5, 9, 15) | |
| val sessionEnd = localInstant(2026, 8, 6, 8, 20) | |
| val range = sleepCleanupRange(sessionStart, sessionEnd, berlin) | |
| assertEquals(sessionStart, range.start) | |
| assertEquals(localInstant(2026, 8, 6, 12, 0), range.end) | |
| } |
🤖 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
`@android/app/src/test/kotlin/wtf/openstrap/openstrap_edge/HealthConnectSleepWriterTest.kt`
around lines 17 - 41, Add a test covering the early-start branch in
sleepCleanupRange, using a session start before local noon on the previous day
so the function returns the session start rather than calculatedStart. Assert
the cleanup range preserves that early start and still uses the expected session
end boundary.
| : types | ||
| .where( | ||
| (type) => | ||
| !_sleepHealthTypes.contains(type) && | ||
| type != HealthDataType.HEART_RATE, | ||
| ) | ||
| .toList(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Align the heart-rate delete predicate with the heart-rate write predicate.
Line 69 removes HealthDataType.HEART_RATE from the delete list when isApplePlatform is false. Line 780 selects the native batch writer with Platform.isAndroid. The two predicates disagree on any platform that is neither Apple nor Android: no heart-rate delete runs, but exportContinuousHeartRateDay falls back to the generic per-sample writes. A repeated export then duplicates heart-rate samples instead of replacing them.
This is the same predicate mismatch that was already fixed for the sleep-stage writes on line 821. Use one predicate for both heart-rate paths.
🐛 Proposed fix
- useAndroidBatch: Platform.isAndroid,
+ useAndroidBatch: !isApple,🤖 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/health/health_export.dart` around lines 65 - 71, Align the heart-rate
delete filter with the native batch-writer selection used by
exportContinuousHeartRateDay: use the same Android/platform predicate at the
heart-rate delete path instead of isApplePlatform. Ensure heart-rate deletion
occurs whenever the generic per-sample write fallback is not used, preventing
repeated exports from duplicating samples.
| if (!shouldAttempt) { | ||
| if (attempts >= _kMaxExportAttempts) return exportBulk(null); | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Run the bulk export when the priority sleep day is only backing off.
Line 418 returns 0 when the priority day is under the attempt cap and not yet due for retry. No bulk export runs in that pass. One failed sleep write therefore stops the export of steps, HRV, energy, workouts, and every older day for the whole backoff tier, which reaches 24 hours.
The give-up branch on line 417 already falls through to exportBulk(null). The under-cap backoff branch is the outlier. During backoff no sleep write is attempted, so no Health Connect quota is reserved for sleep and withholding the bulk export gains nothing.
This also contradicts the retry-cursor design note on lines 306-321, which states that the pipeline must not be wedged indefinitely on one bad day.
🐛 Proposed fix
if (!shouldAttempt) {
- if (attempts >= _kMaxExportAttempts) return exportBulk(null);
- return 0;
+ // Not due for retry (or capped): the sleep write is not
+ // attempted this pass, so it holds no Health Connect quota —
+ // let every other day/metric proceed rather than stalling the
+ // whole pipeline behind one failing night.
+ return exportBulk(null);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!shouldAttempt) { | |
| if (attempts >= _kMaxExportAttempts) return exportBulk(null); | |
| return 0; | |
| } | |
| if (!shouldAttempt) { | |
| // Not due for retry (or capped): the sleep write is not | |
| // attempted this pass, so it holds no Health Connect quota — | |
| // let every other day/metric proceed rather than stalling the | |
| // whole pipeline behind one failing night. | |
| return exportBulk(null); | |
| } |
🤖 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/health/health_export.dart` around lines 416 - 419, Update the
!shouldAttempt branch in the export flow so the under-cap backoff case also
calls exportBulk(null) instead of returning 0. Preserve the existing attempts >=
_kMaxExportAttempts fallback and ensure priority-day backoff still proceeds with
bulk export while skipping the sleep retry.
| var success = true; | ||
| for (final sample in samples) { | ||
| try { | ||
| if (!await writeGeneric( | ||
| sample, | ||
| sample.time.add(const Duration(minutes: 1)), | ||
| )) { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Clamp the Apple sample interval at the export boundary.
A sample in the final minute passes the [start, end) filter. Line 110 then creates an interval after end. This can write data into the next export window.
Clamp the generic write end to end. Add a regression test with a sample at end - 30 seconds.
Proposed fix
var success = true;
for (final sample in samples) {
+ final sampleEnd = sample.time.add(const Duration(minutes: 1));
try {
if (!await writeGeneric(
sample,
- sample.time.add(const Duration(minutes: 1)),
+ sampleEnd.isAfter(end) ? end : sampleEnd,
)) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var success = true; | |
| for (final sample in samples) { | |
| try { | |
| if (!await writeGeneric( | |
| sample, | |
| sample.time.add(const Duration(minutes: 1)), | |
| )) { | |
| var success = true; | |
| for (final sample in samples) { | |
| final sampleEnd = sample.time.add(const Duration(minutes: 1)); | |
| try { | |
| if (!await writeGeneric( | |
| sample, | |
| sampleEnd.isAfter(end) ? end : sampleEnd, | |
| )) { |
🤖 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/health/health_heart_rate_batch.dart` around lines 105 - 111, Update the
sample export loop around writeGeneric to clamp each sample’s computed end time
to the export end boundary, while preserving the existing one-minute interval
for earlier samples. Add a regression test covering a sample at end minus 30
seconds and verify the written interval does not extend beyond end.
| Future<int> healthSyncNow() => _runHealthExport(forceRetry: true); | ||
|
|
||
| Future<int> _runHealthExport({bool forceRetry = false}) => | ||
| _healthExportSingleFlight.run( | ||
| () => _healthExport.exportAll(forceRetry: forceRetry), | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
A manual sync can silently lose forceRetry when it joins an in-flight automatic export.
_healthExportSingleFlight.run returns the existing in-flight future when one exists. healthSyncNow() therefore joins an automatic _runHealthExport() pass that was started with forceRetry: false. The forced pass never runs. Days that are still inside their backoff window stay skipped, and healthSyncNow returns the day count of the non-forced run, so the UI reports a successful manual sync.
_afterDrain calls _runHealthExport() on every light and heavy derive pass, so this overlap is common rather than rare.
Chain a forced pass after the joined run when the caller asked for forceRetry and the in-flight run was not forced. Add a regression test that starts a non-forced export, calls healthSyncNow() while it is in flight, and asserts that a forced exportAll still runs.
🛡️ Proposed direction
- Future<int> _runHealthExport({bool forceRetry = false}) =>
- _healthExportSingleFlight.run(
- () => _healthExport.exportAll(forceRetry: forceRetry),
- );
+ /// A forced (user-initiated) export must not be satisfied by joining a
+ /// non-forced pass already in flight — that pass honours the per-day
+ /// backoff, which is exactly what "Sync now" exists to bypass.
+ bool _healthExportForcedInFlight = false;
+
+ Future<int> _runHealthExport({bool forceRetry = false}) async {
+ final joinedNonForced = forceRetry && !_healthExportForcedInFlight;
+ final joined = await _healthExportSingleFlight.run(() {
+ _healthExportForcedInFlight = forceRetry;
+ return _healthExport
+ .exportAll(forceRetry: forceRetry)
+ .whenComplete(() => _healthExportForcedInFlight = false);
+ });
+ if (!joinedNonForced) return joined;
+ return _healthExportSingleFlight.run(() {
+ _healthExportForcedInFlight = true;
+ return _healthExport
+ .exportAll(forceRetry: true)
+ .whenComplete(() => _healthExportForcedInFlight = false);
+ });
+ }As per coding guidelines: "Behavior changes, especially regressions involving readiness, abstention, idempotence, synchronization, migrations, and lifecycle safety, must include regression tests."
🤖 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 391 - 396, Update _runHealthExport and
its single-flight coordination so a forceRetry: true caller joining an in-flight
non-forced export chains a subsequent forced _healthExport.exportAll run and
returns that forced result; preserve single-flight behavior for matching
requests. Add a regression test covering an in-flight non-forced export followed
by healthSyncNow(), asserting exportAll is invoked again with forceRetry
enabled.
Source: Coding guidelines
`HealthConnectSleepSessionExporter.replace` returned FALSE when a day had a
valid sleep window but no stages -- and the caller treats false as a hard
failure of the ENTIRE day. `health_export.dart` sets `success = false`, which
stops the export cursor advancing, so steps, calories, heart rate and every
other unrelated metric for that day are withheld and retried on backoff
because one hypnogram was missing.
The asymmetry is the tell, three lines apart:
if (session == null) return true; // no window at all -> fine
if (session.stages.isEmpty) return false; // window, no stages -> FAIL
Both are "nothing to write here". Only one said so.
This is not a corner case. Days without staging are ordinary:
* an IMPORTED day (NOOP / WHOOP CSV) carries a sleep window but no
per-second substrate to stage from -- so imported days could never
complete a Health Connect export at all, for anything;
* a night where staging failed keeps its window too.
Deliberately conservative: this does NOT invent a stage-less
SleepSessionRecord, it only stops a missing hypnogram failing everything
else. Writing the bare session span, so imported days still contribute sleep
DURATION, is a genuine improvement -- but it depends on how Health Connect
handles a stage-less record, so it belongs in its own change verified on a
device rather than guessed at here.
One existing test pinned the old return value; updated, keeping its real
assertion (an empty hypnogram must never delete native sleep data) intact and
recording WHY the value flipped. Two tests added for the case that actually
broke: an imported-shaped day with no `series` at all, and the no-window /
no-stages symmetry that was the bug.
NOT fixed here, flagged instead: `delete()` returning false is also treated as
a hard failure, and a no-op delete (nothing of ours to remove -- a first-ever
export) is indistinguishable from a real failure at that API, since the plugin
returns a bare bool for both. Changing it would risk masking genuine write
failures, so I would rather not guess at it without knowing the plugin's
semantics on a real device. Left as-is and raised on the PR.
3 tests added/updated, mutation-verified (restoring `return false` fails
exactly those three). Suite 1223 passing; the 6 failures in
notification_dedupe_test are pre-existing and reproduce on origin/main
unmodified.
|
Reviewed and pushed A missing hypnogram failed the whole day's export
The asymmetry is the tell — three lines apart: if (session == null) return true; // no window at all -> fine
if (session.stages.isEmpty) return false; // window, no stages -> FAILBoth are "nothing to write here". Only one said so. And it isn't a corner case. Days without staging are ordinary:
Deliberately conservativeThis does not invent a stage-less On the test I changedOne existing test pinned the old return value ( Flagged, NOT fixed —
|
|
Checked the bot findings against my change, because one of them looks like it contradicts it. Copilot on
|
|
Correction to my earlier note on this PR, and it's good news. I described the 6 They are a time bomb, not a standing breakage. The suite builds date-prefixed dedupe keys from a hardcoded
Proved it by substituting today's date into the unmodified file on Practical impact here: this PR's CI cannot go green until #207 merges, regardless of its own content. Sorry for the noise — "pre-existing and unrelated" was accurate but undersold that it was actively blocking you. |
Summary
SleepSessionRecordcontaining all normalized stagesfalseresults from Health Connect writes and allow a manual sync to retry capped/backed-off exportsRoot cause
OpenStrap called
health 11.1.1's genericwriteHealthData()once per hypnogram segment. On Android, that package maps everySLEEP_*call to a separateSleepSessionRecordcontaining one stage. Health Connect therefore received fragmented parent sessions instead of one parent record containing the complete hypnogram.The installed dependency does not expose an aggregate sleep-session API, so this adds a small project-local Android
MethodChanneldedicated to replacing one typed sleep session with all of its stages.Validation
flutter test test/health_sleep_export_test.dart --reporter expanded— 7/7 passedflutter analyze— no issues foundflutter build apk --release— succeeded23:55–07:46Edge session with all awake, REM, light, and deep stagesThe full Windows test suite still has three unrelated pre-existing/environmental failures: two DST tests that rely on POSIX timezone mutation and the timing-sensitive
DeriveSchedulerworkout reliability test. The focused regression suite is green.Fixes #193
Summary by CodeRabbit
New Features
Bug Fixes