correlations that read numbers, blood work in your profile, backups, naps you can correct, and breathing you can do with your eyes shut - #221
Conversation
…naps you can correct, and breathing you can do with your eyes shut Five things, in the order they became possible. The typed journal fields shipped with nowhere to go — recorded, displayed, correlated with nothing. They correlate now, on a rank statistic with a slope in the outcome's own units, so the finding reads "about 4 ms less HRV per extra coffee" rather than a coefficient nobody can act on. Kept apart from the tag findings: a tag says those days were different, a dose says more of this goes with less of that, and they carry different evidence. Weight, height and — on Apple, where the records exist — sex and date of birth can be pulled from the health store. Weight is the one that drifts, and three things the app computes depend on it. Weight and height are adopted because they change; age and sex only fill a gap, because they do not, and another app's record does not get to overrule something you set on purpose. Automatic backup, into a folder you can reach from Files and point iCloud or Nextcloud at. It runs when you open the app rather than claiming a schedule the OS will not honour, and it says so. Naps you can correct. The detector's answer is a proposal now: log one it missed, delete one it invented. Edits are stored apart from the detection and replayed over it, so a better detector later still respects "there was no nap here", and a deletion matches by overlap rather than exact bounds, because those bounds move between runs. Logged naps count toward sleep totals, sleep debt and readiness exactly as detected ones do, which is why this bumps the algorithm version. Breathing gained the three things it was missing. Four patterns instead of one, a session that ends when the button said it would, and the strap buzzing each phase change so you can close your eyes — which was always the point of an exercise you are not supposed to be staring at a phone during. Sessions are kept now, so the coherence score is a trend rather than a number that vanished. Coherence stays on resonance breathing only; scoring box breathing against it would grade it on an exam it is not sitting. The interval timer rides the same phase engine, because rounds and breaths are the same problem. It records nothing — a workout already covers that.
|
Warning Review limit reached
Next review available in: 10 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 ignored due to path filters (1)
📒 Files selected for processing (26)
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds persisted nap editing, scheduled local backups, health-profile import, numeric journal insights, configurable paced breathing, and an interval timer. It updates database schemas, platform permissions, profile and sleep screens, workout navigation, and related tests. ChangesSleep nap editing
Profile, backup, and journal data
Breathing and interval timing
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant AppState
participant AutoBackup
participant LocalDb
participant BackupDirectory
AppState->>AutoBackup: runBackupIfDue()
AutoBackup->>LocalDb: exportCopy()
LocalDb-->>AutoBackup: database copy
AutoBackup->>BackupDirectory: store and prune backup
AutoBackup-->>AppState: backup outcome
AppState->>AppState: persist successful run timestamp
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
PR Reviewer Guide 🔍(Review updated until commit baab763)Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Latest suggestions up to baab763 Explore these optional code suggestions:
Previous suggestionsSuggestions up to commit db30204
Suggestions up to commit e61c96b
Suggestions up to commit 1013784
Suggestions up to commit f707b6b
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
test/calm_breathing_view_test.dart (1)
33-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the new pattern and duration selection behavior.
This file still tests only the pre-existing states. The PR adds pattern chips, duration chips, coherence suppression for non-resonance patterns, and automatic stop at the session deadline. None of these are asserted. Add widget tests for at least these cases:
- Selecting a non-resonance pattern hides the coherence readout and shows the "No coherence score for this pattern" text.
- Selecting a pattern and tapping start passes that pattern to
onStart.- Selecting "Open" changes the button text to "Begin".
Do you want me to generate these tests?
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 `@test/calm_breathing_view_test.dart` around lines 33 - 43, Extend the widget tests around CalmBreathingView to cover the new pattern and duration selection behavior: verify a non-resonance pattern hides the coherence readout and displays “No coherence score for this pattern,” selecting a pattern passes it to onStart when starting, and selecting “Open” changes the action button text to “Begin.”Source: Coding guidelines
lib/data/local_repository_impl.dart (1)
2764-2974: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd regression coverage for numeric journal insights.
Add tests for numeric-only journal days and for tag history below four days. Verify that
numeric_insightsstill returns results. Verify custom and deleted field labels use the expected fallback.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/data/local_repository_impl.dart` around lines 2764 - 2974, Add regression tests covering the journal insights flow around _numericJournalInsights: verify numeric-only journal days still produce numeric_insights, tag history with fewer than four days preserves those results, and custom or deleted fields use the expected label fallback. Keep assertions focused on numeric_insights contents and field_label behavior.Source: Coding guidelines
lib/data/db.dart (1)
3720-3740: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
sleep_napis missing from the cross-device import table list.
importFromDbFile'stableslist adds'breathing_session'(line 3735), but not'sleep_nap'. Both tables are new in this PR. Importing another device's backup throughimportFromDbFilesilently drops every nap edit from the source device: the receiving device's merged nap list reverts to the raw detector output on the next derivation of each affected day, with no error surfaced.🐛 Proposed fix
'lab_result', 'lab_marker_def', 'breathing_session', + 'sleep_nap', 'cycle_log', 'notifications', 'baselines', 'sync_cursor', ];Also verify
exportDaysDb's per-day copy loop and its destinationonCreate(around lines 3408 and 3578): neither creates nor copiessleep_nap, so a selected-days export can't carry nap edits either.🤖 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/data/db.dart` around lines 3720 - 3740, Update the importFromDbFile tables list to include sleep_nap alongside the other imported tables. Also update exportDaysDb’s per-day copy loop and destination onCreate schema so sleep_nap is created and copied for selected-day exports, preserving nap edits across device backups.lib/compute/derivation_engine.dart (1)
4382-4523: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy liftNap edits are dropped whenever the detector abstains.
_attachNapscallsapplyNapEdits(detected, napEdits)only on the detector-success path (line 4484). The three abstention/error exits returnnullbefore ever callingapplyNapEdits:
n < 60("too little 1 Hz data"), lines 4398-4401.!m.present, lines 4425-4434.- the
catch (e)block, lines 4518-4522.A manually-logged nap does not depend on 1 Hz data or on the detector succeeding. It is a time window the user typed in. On a day where the detector can't run or abstains — exactly the case this feature exists for ("I took a two-hour nap and it wasn't counted") — the user's manually logged nap is silently discarded, and
nap_minnever credits it. The same applies to a rejected edit, though that case matters less since there is nothing detected to reject.Route every exit through
applyNapEdits, not only the success path. One approach: builddetected(empty on abstention) first, then always merge, and only fall back to_writeUnknownNapswhen the merged result is also empty.🐛 Illustrative fix for the `n < 60` branch (apply the same pattern to the other two exits, ideally via a shared helper)
final n = s.length; if (n < 60) { - _writeUnknownNaps(bundle, 'too little 1 Hz data to assess naps'); - return null; + final merged = applyNapEdits(const [], napEdits); + if (merged.isEmpty) { + _writeUnknownNaps(bundle, 'too little 1 Hz data to assess naps'); + return null; + } + bundle['naps'] = <String, dynamic>{ + 'value': merged, + 'count': merged.length, + 'confidence': 0, + 'tier': 'ESTIMATE', + 'inputs_used': const <String>[], + 'note': 'too little 1 Hz data to assess naps (edited)', + }; + scMap?['nap_min'] = napMinutes(merged).toDouble(); + return [ + for (final nap in merged) + { + 'is_main': false, + 'onset_ts': nap['start'], + 'wake_ts': nap['end'], + 'duration_min': nap['duration_min'], + 'in_bed_min': nap['in_bed_min'], + 'efficiency': nap['efficiency'], + 'confidence': nap['confidence'], + if (nap['source'] != null) 'source': nap['source'], + }, + ]; }This is also an abstention-behavior change with no regression test. Add a test (via
debugAttachNaps) that exercisesn < 60and!m.presentwith a non-emptynapEditslist and asserts the manual nap survives.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/compute/derivation_engine.dart` around lines 4382 - 4523, Update _attachNaps so every exit, including the n < 60 branch, the !m.present branch, and the catch block, applies napEdits to an empty detected list before returning. Reuse a shared merge/finalization path that updates bundle naps, nap_min, and returned nap data consistently; only write unknown naps when the merged result remains empty. Add debugAttachNaps regression tests covering both insufficient data and detector abstention with non-empty napEdits, asserting the manual nap is preserved.Source: Coding guidelines
🤖 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/data/auto_backup.dart`:
- Around line 93-99: Update sortBackupsNewestFirst to accept only filenames
matching the complete format produced by backupFileName, rather than any
openstrap-*.db file. In test/auto_backup_test.dart lines 159-169, add
openstrap-notes.db and assert it is neither selected by the retention logic nor
deleted.
- Around line 130-175: Update runBackupIfDue and the backup coordination around
runBackup so due evaluation, export, unique destination-name allocation,
pruning, and successful last-run persistence execute through one serialized
coordinator; do not rely on the caller’s stale lastRun during concurrent or
cadence-changing triggers. Make backupFileName allocation collision-free for
runs within the same minute, preserve skipped outcomes for backups that are not
due, and add a regression test covering concurrent triggers.
- Around line 45-48: Update the BackupCadence interval logic in
lib/data/auto_backup.dart:45-48 to use local day-label comparisons via
todayLabel() or dayLabelOf() from data/day_label.dart instead of fixed Duration
values, while preserving absolute epoch timestamps. Add daily and weekly
DST-boundary regression cases in test/auto_backup_test.dart:41-77 that verify
cadence behavior across local calendar days without assuming each day lasts
86400 seconds.
- Around line 121-126: Update backupDirectory() to use a user-accessible shared
Documents, Downloads, or Storify location on Android instead of
getApplicationDocumentsDirectory(), applying the required scoped-storage
handling while preserving the existing directory creation behavior. Also update
the temporary-file move flow to copy and then remove the source when a direct
rename cannot cross filesystems.
In `@lib/health/health_profile_import.dart`:
- Around line 117-128: Update requestPermission() and the Android manifest to
declare, check, and request Health Connect history permission using the Health
package API when supported, alongside the existing type-specific read
permissions. Update read() so ten-year queries are performed only when history
access is granted; otherwise constrain the query to Health Connect’s permitted
30-day window, and add regression coverage for an Android metric older than 30
days.
In `@lib/ui/journal/journal_screen.dart`:
- Around line 437-446: Update the methodology text associated with the InfoDot
near the journal insights section to state that insights use both tagged days
and numeric journal entries, rather than tagged days only. Preserve the existing
text styling and surrounding insight-card rendering.
In `@lib/ui/stress/calm_breathing_screen.dart`:
- Around line 105-115: Replace the duplicated _defaultPattern definition with
the resonance entry from kBreathPatterns in breath_phases.dart, and update the
widget’s pattern handling so the default is resolved from that table in
initState rather than requiring a const default. Preserve explicit
widget.pattern values and use the shared entry, including its description and
phase timings, when no pattern is provided.
- Around line 182-201: Add a one-shot completion latch to the state containing
_onTick, check it before invoking widget.onStop, and set it immediately when
remaining reaches zero so asynchronous parent updates cannot trigger repeated
stops. Reset the latch when a new breathing session starts, alongside the
existing timer/elapsed-state initialization.
In `@lib/ui/stress/interval_timer_screen.dart`:
- Around line 59-65: Update dispose to invoke the existing _stop cleanup before
disposing the ticker, ensuring the screen wake lock is released and the clock is
stopped when the screen is removed while running. Preserve the listener removal,
ticker disposal, and super.dispose() calls.
- Around line 100-113: Update the completion branch in the interval timer screen
so the three work-phase buzzes remain fire-and-forget and are dispatched at
fixed millisecond gaps via a Timer.periodic completion handler, without awaiting
or sequentially delaying buzzBreathPhase. Preserve the existing _stop() and
return behavior, and cancel the periodic timer after all three cues are sent.
In `@lib/ui/workouts/workouts_screen.dart`:
- Around line 104-112: Update the onTap callback for IntervalTimerScreen to
guard the outer context after Navigator.pop before pushing the route. Either
capture the navigator before popping and reuse it, or check context.mounted
before Navigator.of(context).push, while preserving the existing themedRoute
configuration.
In `@test/auto_backup_test.dart`:
- Around line 159-169: Add the look-alike file openstrap-notes.db to the
sortBackupsNewestFirst test fixtures and update the expected filenames so only
the valid timestamped backup remains in the returned set, verifying the
look-alike is excluded.
---
Outside diff comments:
In `@lib/compute/derivation_engine.dart`:
- Around line 4382-4523: Update _attachNaps so every exit, including the n < 60
branch, the !m.present branch, and the catch block, applies napEdits to an empty
detected list before returning. Reuse a shared merge/finalization path that
updates bundle naps, nap_min, and returned nap data consistently; only write
unknown naps when the merged result remains empty. Add debugAttachNaps
regression tests covering both insufficient data and detector abstention with
non-empty napEdits, asserting the manual nap is preserved.
In `@lib/data/db.dart`:
- Around line 3720-3740: Update the importFromDbFile tables list to include
sleep_nap alongside the other imported tables. Also update exportDaysDb’s
per-day copy loop and destination onCreate schema so sleep_nap is created and
copied for selected-day exports, preserving nap edits across device backups.
In `@lib/data/local_repository_impl.dart`:
- Around line 2764-2974: Add regression tests covering the journal insights flow
around _numericJournalInsights: verify numeric-only journal days still produce
numeric_insights, tag history with fewer than four days preserves those results,
and custom or deleted fields use the expected label fallback. Keep assertions
focused on numeric_insights contents and field_label behavior.
In `@test/calm_breathing_view_test.dart`:
- Around line 33-43: Extend the widget tests around CalmBreathingView to cover
the new pattern and duration selection behavior: verify a non-resonance pattern
hides the coherence readout and displays “No coherence score for this pattern,”
selecting a pattern passes it to onStart when starting, and selecting “Open”
changes the action button text to “Begin.”
🪄 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: 316466e8-65ec-4e91-9330-4254fa6b2257
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (26)
android/app/src/main/AndroidManifest.xmlios/Runner/Info.plistlib/app.dartlib/compute/derivation_engine.dartlib/compute/nap_edits.dartlib/data/auto_backup.dartlib/data/db.dartlib/data/local_repository_impl.dartlib/health/health_profile_import.dartlib/state/app_state.dartlib/state/prefs.dartlib/ui/journal/journal_screen.dartlib/ui/profile/profile_screen.dartlib/ui/sleep/sleep_periods_screen.dartlib/ui/stress/breath_phases.dartlib/ui/stress/calm_breathing_screen.dartlib/ui/stress/interval_timer_screen.dartlib/ui/workouts/workout_types.dartlib/ui/workouts/workouts_screen.dartpubspec.yamltest/auto_backup_test.darttest/breath_phases_test.darttest/calm_breathing_view_test.darttest/db_migration_ladder_test.darttest/health_profile_import_test.darttest/nap_edits_test.dart
| Duration? get interval => switch (this) { | ||
| BackupCadence.off => null, | ||
| BackupCadence.daily => const Duration(days: 1), | ||
| BackupCadence.weekly => const Duration(days: 7), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use local calendar boundaries for backup cadence.
The implementation and tests model local days as fixed 24-hour durations. DST makes that assumption false.
lib/data/auto_backup.dart#L45-L48: replace fixedDuration(days: ...)cadence checks with local day-label comparisons.test/auto_backup_test.dart#L41-L77: add daily and weekly DST-boundary regression cases.
As per coding guidelines, “Use todayLabel() or dayLabelOf() from data/day_label.dart for local day labels; do not derive labels from UTC strings. Keep epoch timestamps absolute and do not assume every day is 86400 seconds.”
📍 Affects 2 files
lib/data/auto_backup.dart#L45-L48(this comment)test/auto_backup_test.dart#L41-L77
🤖 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/data/auto_backup.dart` around lines 45 - 48, Update the BackupCadence
interval logic in lib/data/auto_backup.dart:45-48 to use local day-label
comparisons via todayLabel() or dayLabelOf() from data/day_label.dart instead of
fixed Duration values, while preserving absolute epoch timestamps. Add daily and
weekly DST-boundary regression cases in test/auto_backup_test.dart:41-77 that
verify cadence behavior across local calendar days without assuming each day
lasts 86400 seconds.
Source: Coding guidelines
| @override | ||
| void dispose() { | ||
| _ticker | ||
| ..removeListener(_onTick) | ||
| ..dispose(); | ||
| super.dispose(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Release the screen wake lock in dispose.
ScreenWake.enable() runs in _start. Only _stop releases it. If the user leaves the screen while the timer runs, dispose runs without _stop, and the wake lock stays held for the rest of the app session. The clock and the ticker also keep the state alive until garbage collection.
Release the wake lock and stop the clock in dispose.
🐛 Proposed fix
`@override`
void dispose() {
+ if (_running) {
+ _clock.stop();
+ ScreenWake.release();
+ }
_ticker
..removeListener(_onTick)
..dispose();
super.dispose();
}📝 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.
| @override | |
| void dispose() { | |
| _ticker | |
| ..removeListener(_onTick) | |
| ..dispose(); | |
| super.dispose(); | |
| } | |
| `@override` | |
| void dispose() { | |
| if (_running) { | |
| _clock.stop(); | |
| ScreenWake.release(); | |
| } | |
| _ticker | |
| ..removeListener(_onTick) | |
| ..dispose(); | |
| super.dispose(); | |
| } |
🤖 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/ui/stress/interval_timer_screen.dart` around lines 59 - 65, Update
dispose to invoke the existing _stop cleanup before disposing the ticker,
ensuring the screen wake lock is released and the clock is stopped when the
screen is removed while running. Preserve the listener removal, ticker disposal,
and super.dispose() calls.
| if (end != null && elapsed >= end) { | ||
| // Three buzzes to say the whole thing is over, distinct from the single | ||
| // cue at a phase change. | ||
| final app = context.read<AppState>(); | ||
| for (final kind in [ | ||
| BreathPhaseKind.work, | ||
| BreathPhaseKind.work, | ||
| BreathPhaseKind.work, | ||
| ]) { | ||
| app.buzzBreathPhase(kind); | ||
| } | ||
| _stop(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect buzzBreathPhase and the underlying strap buzz implementation for coalescing or in-flight guards.
set -euo pipefail
rg -n -C 15 'void buzzBreathPhase' --type=dart lib
rg -n -C 10 -P '\b(buzzPattern|Future<[^>]*>\s+buzz)\s*\(' --type=dart libRepository: OpenStrap/edge
Length of output: 11680
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Locate engine class and send implementation"
rg -n 'class .*Engine|abstract class .*Engine|Future<void> _send|Cmd\.runHapticsPattern|_last.*Buzz|buzz.*inflight|canWrite|writeWithoutResponse' --type=dart lib state
echo
echo "## Suggested slices around buzz calls and send"
for f in lib/ble/ble_engine.dart lib/state/app_state.dart; do
if [ -f "$f" ]; then
echo "### $f"
wc -l "$f"
nl -ba "$f" | sed -n '2500,3190p'
fi
done
echo
echo "## Interval timer slice"
if [ -f lib/ui/stress/interval_timer_screen.dart ]; then
nl -ba lib/ui/stress/interval_timer_screen.dart | sed -n '80,125p'
fiRepository: OpenStrap/edge
Length of output: 1422
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Relevant app_state slice"
nl -ba lib/state/app_state.dart | sed -n '3938,3956p'
echo
echo "## Relevant bleach_engine buzz/write slices"
for s in 280,316,300,250,220,180; do
nl -ba lib/ble/ble_engine.dart | sed -n "\\$((BL_START=${s%100+21})),\\$((BL_END=${s+35})p"
done
echo
echo "## Search BLE write/send guards and haptic definitions"
rg -n -C 8 '(runHapticsPattern|_send\(|writeWithoutResponse|writeCharacteristic|_lastAnyBuzzMs|inflight|busy|debounce|_hapticShortPulse|_last.*BuzzMs|buzz' lib/ble lib/notify lib/state app_state.dart | sed -n '1,260p'Repository: OpenStrap/edge
Length of output: 220
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Relevant app_state slice"
awk '{printf "%6d ", NR} {print}' lib/state/app_state.dart | sed -n '3938,3956p'
echo
echo "## Relevant ble_engine slices"
awk '{printf "%6d ", NR} {print}' lib/ble/ble_engine.dart | sed -n '300,350p'
echo
echo "## Search BLE write/send and haptic-related code"
rg -n -C 8 '(runHapticsPattern|_send\(|writeWithoutResponse|writeCharacteristic|_lastAnyBuzzMs|inflight|busy|debounce|buzz|last.*Buzz)' lib/ble lib/notify lib/state | sed -n '1,260p'Repository: OpenStrap/edge
Length of output: 24456
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## All Cmd.runHapticsPattern references"
rg -n -C 8 'Cmd\.runHapticsPattern|runHapticsPattern|buzzPattern|int pattern|int lastBuzz|_last.*BuzzMs|lastBuzz' lib
echo
echo "## app_state notification relay initialization"
awk '{printf "%6d ", NR} {print}' lib/state/app_state.dart | sed -n '150,185p'
echo
echo "## notification_relay buzz dispatch"
awk '{printf "%6d ", NR} {print}' lib/notify/notification_relay.dart | sed -n '1,230p'
echo
echo "## interval timer completion block (if present)"
if [ -f lib/ui/stress/interval_timer_screen.dart ]; then
awk '{printf "%6d ", NR} {print}' lib/ui/stress/interval_timer_screen.dart | sed -n '90,120p'
fiRepository: OpenStrap/edge
Length of output: 26359
Keep the completion cue fire-and-forget instead of waiting on sequential buzzes.
buzzBreathPhase intentionally never awaits the BLE write; adding timing between the three calls or awaiting them here risks stalling the frame/timer. Send three buzzBreathPhase(BreathPhaseKind.work) calls at fixed millisecond gaps from a Timer.periodic completion handler, or expose a dedicated completion haptic pattern if the engine adds one.
🤖 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/ui/stress/interval_timer_screen.dart` around lines 100 - 113, Update the
completion branch in the interval timer screen so the three work-phase buzzes
remain fire-and-forget and are dispatched at fixed millisecond gaps via a
Timer.periodic completion handler, without awaiting or sequentially delaying
buzzBreathPhase. Preserve the existing _stop() and return behavior, and cancel
the periodic timer after all three cues are sent.
| onTap: () { | ||
| Navigator.pop(ctx); | ||
| Navigator.of(context).push( | ||
| themedRoute( | ||
| (_) => const IntervalTimerScreen(), | ||
| name: 'IntervalTimerScreen', | ||
| ), | ||
| ); | ||
| }, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Guard the outer context after Navigator.pop.
The callback pops the sheet and then uses the outer context to push a route. No await separates the two statements, so this works today. The coding guidelines still require a mounted guard on context use after navigation. Capture the navigator before the pop, or check context.mounted.
♻️ Proposed fix
onTap: () {
+ final nav = Navigator.of(context);
Navigator.pop(ctx);
- Navigator.of(context).push(
+ nav.push(
themedRoute(
(_) => const IntervalTimerScreen(),
name: 'IntervalTimerScreen',
),
);
},As per coding guidelines: "After await or navigation, guard context and UI operations with mounted checks".
📝 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.
| onTap: () { | |
| Navigator.pop(ctx); | |
| Navigator.of(context).push( | |
| themedRoute( | |
| (_) => const IntervalTimerScreen(), | |
| name: 'IntervalTimerScreen', | |
| ), | |
| ); | |
| }, | |
| onTap: () { | |
| final nav = Navigator.of(context); | |
| Navigator.pop(ctx); | |
| nav.push( | |
| themedRoute( | |
| (_) => const IntervalTimerScreen(), | |
| name: 'IntervalTimerScreen', | |
| ), | |
| ); | |
| }, |
🤖 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/ui/workouts/workouts_screen.dart` around lines 104 - 112, Update the
onTap callback for IntervalTimerScreen to guard the outer context after
Navigator.pop before pushing the route. Either capture the navigator before
popping and reuse it, or check context.mounted before
Navigator.of(context).push, while preserving the existing themedRoute
configuration.
Source: Coding guidelines
getApplicationDocumentsDirectory resolves to app-private storage on Android, so backups were being written where no file manager and no sync app could reach them — the feature worked and was pointless. Android writes to app-specific external storage now, which is browsable and needs no permission, and the move handles the two being different filesystems. Retention matched openstrap-*.db, which would have deleted someone's own openstrap-notes.db sitting in the same folder. It matches the exact filename shape now. Filenames carry seconds, because two runs in one minute shared a name and the second overwrote the first, and a backup cannot start while one is already running. Health Connect caps third-party reads at 30 days without a history permission the pinned health package cannot request, so asking Android for ten years returned 30 days while implying otherwise. It asks for what it can have. The finite breathing session called stop on every frame between the timer expiring and the async stop landing — sixty a second, each banking a session row. Latched. The screen also kept its own copy of the resonance pattern, which could drift from the table and shipped an empty description that rendered as a blank line. The interval timer leaked its wake lock when the screen was popped mid-round. And the journal insight note still said tagged days only.
|
Nine of the twelve taken, and two of them were things that would have shipped broken. The Android backup folder was unreachable. The finite session called stop on every frame. Also taken: retention matched One declined. The cadence check uses a 24-hour interval rather than local calendar-day boundaries. The guideline about not assuming 86400-second days is about day labels, where it is exactly right — but this is a "how often", not a date, and an interval is the better model for it: a backup at 23:00 followed by one at 00:30 should not count as two days. On a DST day the effect is that a daily backup runs an hour earlier or later, never that one is skipped, because the trigger is foreground rather than a fixed alarm. Using calendar days here would introduce the skip it is meant to prevent. |
|
Persistent review updated to latest commit c7c949f |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/data/auto_backup.dart (1)
142-152: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftUse shared Android storage for user-facing backups.
getExternalStorageDirectory()maps to AndroidgetExternalFilesDir(null). Android 11 scoped storage prevents other apps from accessing an app-specific external directory. A file manager or sync application cannot reliably use this backup folder. Store backups in shared Documents or Downloads through MediaStore or a persisted SAF location. Handle revoked access explicitly. (developer.android.com)🤖 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/data/auto_backup.dart` around lines 142 - 152, Update backupDirectory() so Android backups use a user-accessible shared Documents or Downloads location via MediaStore or a persisted Storage Access Framework location instead of getExternalStorageDirectory(). Handle missing or revoked persisted access explicitly, and retain the application-documents fallback only when no valid shared location is available.
🤖 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/data/auto_backup.dart`:
- Around line 166-175: Refactor the coordination around runBackup and the
related due-check, export, destination-allocation, pruning, and lastRun
persistence paths so a single coordinator owns the entire backup lifecycle and
keeps _inFlight active until successful timestamp persistence completes. Ensure
destination names remain unique for same-second manual requests, and add
regression coverage for a trigger between export completion and timestamp
persistence plus two same-second manual requests, asserting the expected single
export and two unique files.
---
Duplicate comments:
In `@lib/data/auto_backup.dart`:
- Around line 142-152: Update backupDirectory() so Android backups use a
user-accessible shared Documents or Downloads location via MediaStore or a
persisted Storage Access Framework location instead of
getExternalStorageDirectory(). Handle missing or revoked persisted access
explicitly, and retain the application-documents fallback only when no valid
shared location is available.
🪄 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: 4121e1ce-d777-4688-9019-7e7cec4c12d5
📒 Files selected for processing (8)
lib/data/auto_backup.dartlib/health/health_profile_import.dartlib/ui/journal/journal_screen.dartlib/ui/stress/calm_breathing_screen.dartlib/ui/stress/interval_timer_screen.darttest/auto_backup_test.darttest/breath_phases_test.darttest/health_profile_import_test.dart
Guarding only the export left a window: a second trigger could read the old last-run timestamp, judge a backup due, and start another one while the first was finishing. Reading the timestamp, deciding, writing and recording are one sequence now, and the caller passes callbacks rather than a value so the read happens inside the lock rather than before queueing. Two runs in the same second also shared a filename, so the first was silently replaced. The destination is allocated against what is already on disk.
|
Right, and I had half-fixed it. The guard covered the export but ended before the caller persisted the timestamp, so a second trigger could still read the old value, judge a backup due, and start another one. The lock covers the whole sequence now — read, decide, export, prune, record. The part that makes it actually work is that Same-second destinations are allocated against what is already on disk, with a test that two runs at an identical timestamp produce two files rather than one overwritten. The queue also survives a failed run, which it needed to — a chained tail without an error guard would have wedged every later backup for the life of the process after one throw. |
|
Persistent review updated to latest commit f707b6b |
|
@CodeRabbit review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/data/auto_backup.dart`:
- Around line 226-233: Update _uniqueDestination so it never returns an existing
candidate: check the base name and suffixes through -99, return immediately only
when a free File is found, and signal a backup error when every bounded
candidate is occupied. Add a regression test covering all candidates from the
base through -99.
In `@lib/state/app_state.dart`:
- Around line 807-814: Update runBackupIfDue so it does not capture
backupCadence before entering backup.runBackupIfDue; pass a BackupCadence
Function() callback that reads the current cadence inside the serialized
operation immediately before backupIsDue, preserving the off behavior there. Add
a queued cadence-change regression test covering a cadence changed to off while
the call waits behind an export.
In `@test/auto_backup_test.dart`:
- Around line 344-359: The queue-recovery test currently skips both backups, so
it never exercises failure handling. Update test one failure does not wedge
every later backup to inject or otherwise use a test seam that makes the first
queued backup fail, then invoke a second backup with an executable cadence and
assert that it runs successfully; preserve the existing verification that the
queue remains usable after the failure.
🪄 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: ed055a1a-751c-4303-81e8-79b3f1f7e6f4
📒 Files selected for processing (3)
lib/data/auto_backup.dartlib/state/app_state.darttest/auto_backup_test.dart
The cadence was read before the call entered the queue, so a call waiting behind a running export acted on the setting as it was when it queued. Someone who switched backup off in the meantime still got another unencrypted copy of their health data written after they had disabled it. It re-reads inside the lock now, like the timestamp already did. The unique-filename search returned its last candidate when every one was taken, handing the next backup a real snapshot to overwrite — the opposite of the point. It returns an error instead. And the queue-recovery test proved nothing: both its calls were cadence off, so neither reached the export and neither could fail. The exporter is injectable now, so a failure is actually reachable, and the test asserts the backup after it still runs.
|
All three, and two of them are mine from the last round. The cadence was the same bug I had just fixed for the timestamp, left in place one line above it. Read before the call entered the queue, so a call waiting behind a running export acted on the setting as it was when it queued — someone switching backup off mid-export would still have got another unencrypted copy of their health data written after disabling it. It re-reads inside the lock now, and the test flips the setting from inside the first export to prove the queued call sees it. The unique-filename search returning its last candidate when all hundred were taken was straightforwardly backwards: it hands the next backup a real snapshot to overwrite, which is the one thing the unique naming exists to prevent. It errors instead, and the test fills every slot and checks all hundred files are still intact afterwards. And you are right that the queue-recovery test proved nothing — both calls were cadence |
|
Persistent review updated to latest commit 1013784 |
|
@coderabbitai review |
|
The detector abstains on short records, partial days and failures — exactly the days someone would log a nap for — and all three of those paths returned before the edits were applied. The nap vanished: no card, no minutes, and no way to delete the row that had just been created, while the edit kept force-re-deriving that day forever. Logged naps carry their own bounds, so they are published on their own now, and the day is only reported as unjudged when nothing was logged. A logged nap also supersedes a detection it overlaps. The entry screen refuses an overlap against what it can see, but that is a snapshot — log one on a day the detector abstained on, sync more raw, and it may then find the same bout and count the afternoon twice. And main sleep is in the overlap check now, or a nap could be logged inside the night and counted in both. Re-entering the breathing screen mid-session left it frozen. Swipe-back never reaches the handler that stops a session, so the next view mounted with the session already running and no state change to catch — no clock, no haptics, and a timed session that never ended. Backgrounding a session banked whatever the wall clock said. Two minutes backgrounded and resumed forty minutes later stored a forty-minute session, with a coherence score drawn mostly from breathing nobody was pacing. It clamps to what was asked for, since the pacer stops the moment the app leaves the foreground. The interval timer's end-of-session cue was three identical buzzes, which the firmware plays as one and nobody could tell from an ordinary round change. It has its own pattern. Restoring a backup dropped every sleep correction — the edits are the only copy, so a restore silently reinstated naps that had been deleted. And the Health import searched ten years back while adopting weight unconditionally, so an ancient reading could overwrite a profile someone had kept current, then feed calories and BMR. A year.
|
CodeRabbit stayed rate limited, so I ran my own adversarial pass over the diff instead. Eight real defects, two of which would have made the features look broken rather than subtly wrong. A nap logged on a day the detector abstained on vanished. All three abstention paths — too little data, detector not present, exception — returned before the edits were applied. So on exactly the days this feature exists for (strap off part of the afternoon, short record) the nap produced no card, credited no minutes, and could not even be deleted afterwards, while the stored edit kept force-re-deriving that day forever. Logged naps carry their own absolute bounds and need nothing from the detector, so they are published on their own now; the day is only reported unjudged when nothing was logged. Re-entering the breathing screen mid-session left it dead. Swipe-back and system back never reach Also fixed: a logged nap now supersedes a detection it overlaps (the entry-time overlap check only sees a snapshot, so a later re-derive could find the same bout and count the afternoon twice); main sleep is in that overlap check, or a nap could be logged inside the night and counted in both; backgrounding a session banked wall-clock time, so two minutes backgrounded and resumed forty minutes later stored a forty-minute session with a coherence score drawn mostly from unpaced breathing; the interval timer's "three buzzes" for session-end are played by the firmware as one and were indistinguishable from a round change, so it has its own pattern; restoring a backup dropped every sleep correction, and since the edits are the only copy that silently reinstated deleted naps; and the Health import searched ten years back while adopting weight unconditionally, letting an ancient reading overwrite a current profile and then feed calories and BMR. Two things noted and deliberately not changed: both ticker screens 1727 tests. |
|
Persistent review updated to latest commit e61c96b |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/ui/stress/interval_timer_screen.dart (1)
78-121: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd interval timer lifecycle regression coverage.
This screen adds wake-lock and haptic lifecycle behavior, but the existing matchers only cover
ScreenWakeandintervalPatternlogic separately. Add coverage for start, phase-boundary haptics, finite completion with the distinctbuzzSessionComplete()cue, open rounds, and wake-lock release on disposal.🤖 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/ui/stress/interval_timer_screen.dart` around lines 78 - 121, Add regression coverage for the interval timer screen’s lifecycle, exercising _start, phase-boundary haptic dispatch, finite-session completion through the distinct buzzSessionComplete() cue, open-ended rounds, and wake-lock release when the screen is disposed. Reuse the existing ScreenWake and intervalPattern test infrastructure and verify start enables wake-lock while stop/completion/disposal releases it.Source: Coding guidelines
lib/state/app_state.dart (1)
3922-3968: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winPersist a duration-consistent
ended_at, not the unclamped wall-clock stop.
secondsis clamped totarget.inSecondsso a session resumed long after backgrounding does not bank suspended time as breathing.endedAt, however, is still stamped from the unclampedended(DateTime.now()), so the persisted row carries two disagreeing measures of the same session:seconds(correct, clamped) versusended_at - started_at(the original unclamped span). A reader that computes duration from the timestamps instead of thesecondscolumn reproduces exactly the "banked a forty-minute session" bug this clamp exists to fix.Derive
endedAtfrom the same clamped value used forsecondsso both fields agree.🛡️ Proposed fix to keep endedAt consistent with the clamped seconds
if (started != null) { final ended = DateTime.now(); var seconds = ended.difference(started).inSeconds; // Clamped to what was asked for. Overshoot is always suspension, never // extra breathing — the pacer stops the moment the app leaves the // foreground, so any second past the target was spent doing something // else. if (target != null && seconds > target.inSeconds) { seconds = target.inSeconds; } if (seconds >= 60) { final res = breathingResult; final scored = res != null && res['ok'] == true; final rated = breathingPattern.coherenceRated && scored; unawaited( LocalDb.putBreathingSession( startedAt: started.millisecondsSinceEpoch, - endedAt: ended.millisecondsSinceEpoch, + endedAt: started.add(Duration(seconds: seconds)).millisecondsSinceEpoch, pattern: breathingPattern.key, seconds: seconds,🤖 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 3922 - 3968, Update stopBreathingSession so the persisted endedAt is derived from the clamped seconds value rather than the raw ended timestamp. Preserve the existing target-based clamping and use the adjusted end time consistently when calling LocalDb.putBreathingSession, ensuring ended_at minus started_at matches seconds.
🤖 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/compute/derivation_engine.dart`:
- Around line 4379-4399: The _napsWhenUnjudged method assigns the unsupported
analytics tier 'reported' to bundle['naps']['tier']; replace it with the
established supported tier constant or value appropriate for user-logged naps,
while preserving the existing reported-input behavior and other metadata.
In `@lib/data/auto_backup.dart`:
- Around line 194-200: Update _runBackup to call _uniqueDestination before
invoking the snapshot export, and return the existing no-free-filename
BackupOutcome immediately when it returns null. Export the snapshot only after a
valid destination is allocated, preventing an unmovable temporary file from
being created.
In `@lib/ui/sleep/sleep_periods_screen.dart`:
- Around line 169-186: Add an immediate mounted check at the start of _rederive,
before context.read<AppState>() or any other operation, returning early when the
widget is disposed so both _addNap and _removeNap callers are safe after their
awaited database writes.
In `@lib/ui/stress/calm_breathing_screen.dart`:
- Around line 143-173: Preserve the active session’s timing across
CalmBreathingView remounts by storing its start time, target duration, or
absolute deadline in AppState rather than resetting it in _begin. Pass that
authoritative timing state into CalmBreathingView, initialize _minutes from it,
and calculate phase, elapsed time, remaining time, and completion against the
persisted value; _begin should only resume the clock without restarting the
session. Add a regression test covering remounting an active five-minute or
open-ended session and verifying its phase and remaining time remain unchanged.
In `@test/health_profile_import_test.dart`:
- Around line 105-110: Update the assertion in the importer test around
windows.single to compare the window start against the exact expected date,
rather than only checking the year difference. Preserve the existing
single-window assertion and verify the Apple window begins on the intended
one-year boundary for DateTime(2026, 8, 9).
---
Outside diff comments:
In `@lib/state/app_state.dart`:
- Around line 3922-3968: Update stopBreathingSession so the persisted endedAt is
derived from the clamped seconds value rather than the raw ended timestamp.
Preserve the existing target-based clamping and use the adjusted end time
consistently when calling LocalDb.putBreathingSession, ensuring ended_at minus
started_at matches seconds.
In `@lib/ui/stress/interval_timer_screen.dart`:
- Around line 78-121: Add regression coverage for the interval timer screen’s
lifecycle, exercising _start, phase-boundary haptic dispatch, finite-session
completion through the distinct buzzSessionComplete() cue, open-ended rounds,
and wake-lock release when the screen is disposed. Reuse the existing ScreenWake
and intervalPattern test infrastructure and verify start enables wake-lock while
stop/completion/disposal releases it.
🪄 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: a43a9bcd-219c-4787-a544-8d436c9e016c
📒 Files selected for processing (13)
lib/compute/derivation_engine.dartlib/compute/nap_edits.dartlib/data/auto_backup.dartlib/data/db.dartlib/health/health_profile_import.dartlib/state/app_state.dartlib/ui/sleep/sleep_periods_screen.dartlib/ui/stress/calm_breathing_screen.dartlib/ui/stress/interval_timer_screen.darttest/auto_backup_test.darttest/calm_breathing_view_test.darttest/health_profile_import_test.darttest/nap_edits_test.dart
…ing it Starting the clock on mount fixed a frozen re-entry and introduced a worse one: the clock restarted from zero and the length reverted to the default, so a five-minute session re-entered at 4:00 showed 2:00 and stopped almost at once. Elapsed and remaining now come from when the session actually began and what it was actually asked for, both of which belong to the session. A logged nap was published under a tier that does not exist. The vocabulary is four values; AUTH is the one that means directly measured or definitional, which is what a self-report is — the user is not estimating that they napped. The backup exported before choosing a destination, so a run that could not find a free name left a full copy of the database in temp, once per attempt. And re-deriving after a nap edit reached for the provider without checking the screen was still there, after two awaited writes.
|
All five, and the deadline one is a defect in the fix from my last round — fourth consecutive round where that has happened. Starting the clock on mount fixed the frozen re-entry and introduced a worse version of the same bug: the clock restarted from zero and the length reverted to the picker's default, so a five-minute session re-entered at 4:00 showed 2:00 and stopped almost immediately. The real problem was that the view owned the timing at all — it is remounted every time someone leaves the screen and comes back. Elapsed and remaining now come from when the session actually began and what it was actually asked for, both held by the session. Regression test asserts a 5-minute session remounted at 4:00 shows 0:59 remaining and does not stop. The tier was a straightforward mistake on my part — The backup exporting before choosing a destination is a good catch on a path I had just added a test for without asserting the cleanup. Destination first now, so a run that cannot find a free name never exports at all — and the test asserts the exporter was never called rather than just that the outcome failed. Also fixed: the |
|
Persistent review updated to latest commit db30204 |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
lib/ui/sleep/sleep_periods_screen.dart (2)
107-112: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBuild the next local calendar time instead of adding 86400 seconds.
endTs += 24 * 3600is wrong on DST transition days. It can shift an overnight nap by one hour and break overlap validation and persisted duration.Proposed fix
- if (endTs <= startTs) endTs += 24 * 3600; + if (endTs <= startTs) { + endTs = DateTime( + day.year, + day.month, + day.day + 1, + end.hour, + end.minute, + ).millisecondsSinceEpoch ~/ + 1000; + }As per coding guidelines, “Keep epoch timestamps absolute and do not assume every day is 86400 seconds.”
🤖 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/ui/sleep/sleep_periods_screen.dart` around lines 107 - 112, Update the overnight adjustment in the sleep-period calculation around endTs and startTs to construct the next local calendar occurrence of the end time rather than adding 24*3600 seconds. Preserve the existing end-before-or-equal-start condition and ensure the resulting epoch timestamp remains correct across DST transitions for overlap validation and persisted duration.Source: Coding guidelines
385-395: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse self-report help text for logged naps.
A nap with
source == 'manual'is not detected from wrist stillness or heart rate. The currentInfoDotstates that it was detected and refers to detection confidence.Render a separate description for logged naps.
🤖 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/ui/sleep/sleep_periods_screen.dart` around lines 385 - 395, Update the InfoDot body in the sleep-period rendering around isMain to distinguish manually logged naps (source == 'manual') from detected naps. Use the self-report help text for logged naps, while preserving the existing detected-nap description for non-manual naps and the main-sleep text.lib/state/app_state.dart (2)
1667-1671: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftQueue a forced re-derive for nap edits.
reanalyzeForNapEdit()uses_reanalyzeForOverride(). That flow returns whenreanalyzingis true.DerivationEngine.run()also returns0when another derivation is active. A nap edit can commit but never replay intoday_result.Queue one coalesced forced pass after the active derivation completes. Add a regression test that saves a nap edit during an active derive and verifies the persisted nap appears afterward.
🤖 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 1667 - 1671, Update reanalyzeForNapEdit() and the shared derivation coordination so a nap edit requested during an active derivation queues one coalesced forced pass that runs after the current derivation completes, rather than being dropped by reanalyzing or DerivationEngine.run(). Add a regression test covering a saved nap edit during an active derive and verify the persisted nap appears in day_result afterward.
3941-3969: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPersist the effective session end time.
When a finite session resumes after its target elapsed,
secondsis clamped butendedAtstill uses the later wall-clock time. The stored row then reports a duration that does not match its start and end timestamps.Set
endedAttostarted + Duration(seconds: seconds)after clamping.Proposed fix
if (target != null && seconds > target.inSeconds) { seconds = target.inSeconds; } + final effectiveEnded = started.add(Duration(seconds: seconds)); if (seconds >= 60) { final res = breathingResult; @@ - endedAt: ended.millisecondsSinceEpoch, + endedAt: effectiveEnded.millisecondsSinceEpoch,🤖 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 3941 - 3969, After clamping seconds in the session-finalization flow, set the persisted end timestamp used by LocalDb.putBreathingSession to started plus Duration(seconds: seconds), so endedAt matches the effective duration rather than the later wall-clock ended value. Keep the existing clamping and persistence behavior otherwise unchanged.lib/ui/stress/calm_breathing_screen.dart (1)
187-194: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not emit a phase cue on a view remount.
An active view calls
_begin()duringinitState._begin()clears_lastPhaseand_lastCycle, so the next frame invokesonPhaseChangefor the current phase even when no boundary occurred.Initialize the phase tracking from
phaseAt(_pattern, _elapsed)when resuming an existing session. Keep the initial cue for a newly started session. Add a remount test that confirms no cue occurs before the next boundary.Also applies to: 236-243
🤖 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/ui/stress/calm_breathing_screen.dart` around lines 187 - 194, Update _begin() so resuming an existing session initializes _lastPhase and _lastCycle from phaseAt(_pattern, _elapsed) instead of clearing them, preventing onPhaseChange until the next boundary. Preserve the initial phase cue for newly started sessions, and add a remount test verifying no cue is emitted before that boundary.Source: Coding guidelines
🤖 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/ui/stress/calm_breathing_screen.dart`:
- Around line 210-214: Update the _target getter to preserve an authoritative
open-ended session when startedAt indicates a remounted running session,
returning null instead of falling back to the draft duration; retain the
existing widget-test fallback when no authoritative session timing exists.
Extend the remount regression test with a past startedAt and target: null,
asserting elapsed time is displayed and onStop is not called.
---
Outside diff comments:
In `@lib/state/app_state.dart`:
- Around line 1667-1671: Update reanalyzeForNapEdit() and the shared derivation
coordination so a nap edit requested during an active derivation queues one
coalesced forced pass that runs after the current derivation completes, rather
than being dropped by reanalyzing or DerivationEngine.run(). Add a regression
test covering a saved nap edit during an active derive and verify the persisted
nap appears in day_result afterward.
- Around line 3941-3969: After clamping seconds in the session-finalization
flow, set the persisted end timestamp used by LocalDb.putBreathingSession to
started plus Duration(seconds: seconds), so endedAt matches the effective
duration rather than the later wall-clock ended value. Keep the existing
clamping and persistence behavior otherwise unchanged.
In `@lib/ui/sleep/sleep_periods_screen.dart`:
- Around line 107-112: Update the overnight adjustment in the sleep-period
calculation around endTs and startTs to construct the next local calendar
occurrence of the end time rather than adding 24*3600 seconds. Preserve the
existing end-before-or-equal-start condition and ensure the resulting epoch
timestamp remains correct across DST transitions for overlap validation and
persisted duration.
- Around line 385-395: Update the InfoDot body in the sleep-period rendering
around isMain to distinguish manually logged naps (source == 'manual') from
detected naps. Use the self-report help text for logged naps, while preserving
the existing detected-nap description for non-manual naps and the main-sleep
text.
In `@lib/ui/stress/calm_breathing_screen.dart`:
- Around line 187-194: Update _begin() so resuming an existing session
initializes _lastPhase and _lastCycle from phaseAt(_pattern, _elapsed) instead
of clearing them, preventing onPhaseChange until the next boundary. Preserve the
initial phase cue for newly started sessions, and add a remount test verifying
no cue is emitted before that boundary.
🪄 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: 05ee70e4-2761-4538-97c8-f2006f664845
📒 Files selected for processing (8)
lib/compute/derivation_engine.dartlib/data/auto_backup.dartlib/state/app_state.dartlib/ui/sleep/sleep_periods_screen.dartlib/ui/stress/calm_breathing_screen.darttest/auto_backup_test.darttest/calm_breathing_view_test.darttest/health_profile_import_test.dart
A null target on a running session means it runs until you stop it. Reading that as "no answer" fell through to the picker's default, so an open session left and re-entered past two minutes showed a countdown it never had and stopped on the spot. The session's start is what says whether a target is authoritative, not whether the target happens to be null.
|
Taken — and it is the same mistake as the round before, one layer down. I keyed "is there an authoritative target" on the target being non-null, but null is itself a meaningful answer for a running session: it means open-ended. So an open session left and re-entered past two minutes inherited the picker's default, showed a countdown it never had, and stopped on the spot. The session's |
|
Persistent review updated to latest commit baab763 |
|
@CodeRabbit review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
lib/state/app_state.dart (3)
3961-3969: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winHandle failures from breathing-history persistence.
LocalDb.putBreathingSession(...)runs withunawaitedand no error handler. If the write fails or the process stops before it completes,stopBreathingSession()returns as if the session was saved and the history row is lost.Await the write before returning, or attach explicit failure handling and retry behavior.
🤖 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 3961 - 3969, Update stopBreathingSession around the unawaited LocalDb.putBreathingSession call to ensure persistence failures are handled before the method returns: preferably await the write, or attach explicit failure handling with the required retry behavior. Preserve the existing breathing-session fields and completion flow while preventing silent loss of history rows.
4009-4023: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winScope coherence to the current resonance-rated session.
_recomputeBreathingCoherence()computes for every pattern, althoughstopBreathingSession()only accepts coherence forcoherenceRatedpatterns. Add an early guard for non-rated patterns.Also capture a session identifier and the pattern before the await. The current
if (!breathingActive)check passes when an old request completes after stop and a new session starts. That request can overwrite the new session’s result and history.🤖 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 4009 - 4023, Update _recomputeBreathingCoherence to return immediately unless breathingPattern.coherenceRated is true. Before awaiting repo!.breathingCoherence, capture the current session identifier and pattern, then only apply breathingResult and related history when both still match the active session and pattern; prevent stale requests from overwriting a newly started session.
1671-1671: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftQueue nap re-analysis requests.
reanalyzeForNapEdit()delegates to_reanalyzeForOverride(), which returns immediately whenreanalyzingis already true. If a second nap edit is saved while the first derive is running, the first derive may have already loadednapEditsand will not include the second edit. The second call then reloads stale derived data, and no later derive is scheduled.Serialize the in-flight future and rerun when an edit arrives during the pass, or prevent edits until the pass completes. Add a regression test for two edits during one derive.
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` at line 1671, Update reanalyzeForNapEdit() and the _reanalyzeForOverride() flow to serialize in-flight nap re-analysis requests and schedule another derive when an edit arrives during the current pass, ensuring both edits are reflected in the final derived data. Add a regression test that saves two nap edits while one derive is running and verifies a subsequent re-analysis processes the second edit.Source: Coding guidelines
lib/ui/sleep/sleep_periods_screen.dart (1)
102-112: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winUse local calendar arithmetic for overnight naps.
Line 112 adds
24 * 3600to the end timestamp. This assumes every local day has 86,400 seconds. On a DST transition date, the stored nap end is one hour wrong.Construct the next local date with
DateTime(day.year, day.month, day.day + 1, ...), then convert both local endpoints to absolute epoch seconds.Proposed fix
- final startTs = - DateTime(day.year, day.month, day.day, start.hour, start.minute) - .millisecondsSinceEpoch ~/ - 1000; - var endTs = - DateTime(day.year, day.month, day.day, end.hour, end.minute) - .millisecondsSinceEpoch ~/ - 1000; + final startAt = + DateTime(day.year, day.month, day.day, start.hour, start.minute); + var endAt = DateTime(day.year, day.month, day.day, end.hour, end.minute); // An end before the start means it ran past midnight. - if (endTs <= startTs) endTs += 24 * 3600; + if (!endAt.isAfter(startAt)) { + endAt = DateTime( + day.year, + day.month, + day.day + 1, + end.hour, + end.minute, + ); + } + final startTs = startAt.millisecondsSinceEpoch ~/ 1000; + final endTs = endAt.millisecondsSinceEpoch ~/ 1000;As per coding guidelines, “Keep epoch timestamps absolute and do not assume every day is 86400 seconds.”
🤖 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/ui/sleep/sleep_periods_screen.dart` around lines 102 - 112, Update the timestamp construction around startTs and endTs to use local calendar DateTime arithmetic for overnight naps: when the end time is not after the start, construct the end endpoint using the next local date via DateTime(day.year, day.month, day.day + 1, ...), then convert both local endpoints to epoch seconds. Remove the fixed 24 * 3600 adjustment so DST transition days retain correct absolute timestamps.Source: Coding guidelines
🤖 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 `@test/calm_breathing_view_test.dart`:
- Around line 107-125: Update the remount test around CalmBreathingView to keep
the stopped assertion separate from the displayed-time check, and accept either
“0:59” or “1:00” for the one-minute remaining value because pump timing may not
cross a wall-clock second. Preserve coverage that the session does not stop
early and does not revert to the default two-minute duration.
---
Outside diff comments:
In `@lib/state/app_state.dart`:
- Around line 3961-3969: Update stopBreathingSession around the unawaited
LocalDb.putBreathingSession call to ensure persistence failures are handled
before the method returns: preferably await the write, or attach explicit
failure handling with the required retry behavior. Preserve the existing
breathing-session fields and completion flow while preventing silent loss of
history rows.
- Around line 4009-4023: Update _recomputeBreathingCoherence to return
immediately unless breathingPattern.coherenceRated is true. Before awaiting
repo!.breathingCoherence, capture the current session identifier and pattern,
then only apply breathingResult and related history when both still match the
active session and pattern; prevent stale requests from overwriting a newly
started session.
- Line 1671: Update reanalyzeForNapEdit() and the _reanalyzeForOverride() flow
to serialize in-flight nap re-analysis requests and schedule another derive when
an edit arrives during the current pass, ensuring both edits are reflected in
the final derived data. Add a regression test that saves two nap edits while one
derive is running and verifies a subsequent re-analysis processes the second
edit.
In `@lib/ui/sleep/sleep_periods_screen.dart`:
- Around line 102-112: Update the timestamp construction around startTs and
endTs to use local calendar DateTime arithmetic for overnight naps: when the end
time is not after the start, construct the end endpoint using the next local
date via DateTime(day.year, day.month, day.day + 1, ...), then convert both
local endpoints to epoch seconds. Remove the fixed 24 * 3600 adjustment so DST
transition days retain correct absolute timestamps.
🪄 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: b5e37937-3c25-4dff-9e64-7a829d675e52
📒 Files selected for processing (8)
lib/compute/derivation_engine.dartlib/data/auto_backup.dartlib/state/app_state.dartlib/ui/sleep/sleep_periods_screen.dartlib/ui/stress/calm_breathing_screen.darttest/auto_backup_test.darttest/calm_breathing_view_test.darttest/health_profile_import_test.dart
1 participant
User description
Five of the six remaining items from the discussions triage. Per-app vibration patterns is parked.
Journal correlations. The typed fields shipped with nowhere to go — recorded, displayed, correlated with nothing. This moves the analytics pin to pick up
journalNumericCorrelationsand wires it in. Findings lead with a slope in the outcome's own units ("about 4 ms less HRV per extra coffee") rather than a coefficient, with rho as supporting detail. Deliberately a separate pass from the tag correlations rather than more rows in the same list: a tag answers "were those days different", a dose answers "does more of this go with less of that", and they carry different evidence — a difference of means with a Cohen's d versus a rank correlation with a confidence interval. Its date axis is the days a number was recorded, which is not the same set as the days a tag was.Profile from the health store. Weight, height, and on Apple sex and date of birth. Weight and height are adopted because they drift and three things the app computes depend on them; age and sex only fill a gap, because neither changes and another app's record should not overrule something you set deliberately. Sex and DOB are requested only on Apple — Health Connect has no characteristic record for either, and requesting them there is issue #184's shape again. Android's manifest gains
READ_WEIGHT/READ_HEIGHTwith no write counterparts; without them Health Connect silently returns nothing, which is indistinguishable from an empty store.Automatic backup. Daily or weekly, into an
OpenStrap Backupsfolder in the app's Documents directory, reachable from Files (UIFileSharingEnabled+LSSupportsOpeningDocumentsInPlace) and from the Android file manager. Point iCloud Drive, Synology or Nextcloud at it. Not a user-picked folder: that needs a persisted SAF tree URI or a security-scoped bookmark, both of which expire silently, and a backup that quietly stopped working is worse than one that lives somewhere slightly less convenient. It runs on foreground when due rather than pretending to a background schedule neither platform will honour, and the sheet says so — along with the fact that it is not encrypted.Naps you can correct. Three people asked, from two directions: "it tracked sleep when I was awake, let me delete it" and "I napped for two hours and it wasn't counted". Same feature — the detector's answer is a proposal and the person who was there gets the last word.
Two decisions worth calling out. Edits are stored separately from the detector's output and replayed over it on every derivation, rather than written into the result: the detector improves, and a day re-derived under a better stager should still respect "there was no nap here" instead of freezing the old detection alongside the edit. And a rejection matches by overlap, not exact bounds — detected boundaries shift between runs, and an edit that stopped applying when one moved by a minute would let a deleted nap quietly come back.
This bumps
kAlgoVersionto 61, and that is the point rather than a side effect: logged naps count toward sleep totals, sleep debt and readiness exactly as detected ones do. Days carrying an edit are force-derived alongside sleep-override days so an edit to a finalized day takes effect.Breathing. The screen already had real cardiac coherence from live RR; it was missing the other three things. Four patterns instead of one fixed pace. A session that ends when the button said it would — it used to say "2-Minute Session" and run until you stopped it. And the strap buzzes each phase change, which was always the point of an exercise you are not supposed to be watching a phone during. Sessions are kept now, so the score is a trend rather than a number that vanished on stop. Coherence stays on resonance only: it measures oscillation at the paced frequency, and scoring box breathing against it grades it on an exam it is not sitting.
Interval timer rides the same phase engine — rounds and breaths are the same problem. It records nothing and starts no session; a workout already covers that, and a timer that quietly created sessions would double-count every round. Reachable from the start-workout sheet, which is only safe because that sheet became scrollable earlier.
Schema 29 → 31 across three additive rungs, with a migration test that walks v27 all the way up and writes to every new table — a CREATE with a typo still leaves a table nothing can write to.
PR Type
Enhancement, Bug fix, Tests
Description
Five new user-facing features: journal numeric correlations, health-store profile import, automatic backup, correctable naps, and improved guided breathing
kAlgoVersionbumped 60→61 andschemaVersionbumped 29→31; two new tables (sleep_nap,breathing_session) added with migrationsBreathing screen gains four patterns, real session timer, per-phase strap buzz, and session history; new interval timer screen shares the same phase engine
Nap edits (add/reject) stored separately from detector output and replayed on every derivation; rejection matches by overlap, not exact bounds
Diagram Walkthrough
File Walkthrough
11 files
Bump kAlgoVersion 60→61; replay nap edits over detector outputSchema v29→31; add sleep_nap and breathing_session tablesPure nap-edit merge logic: applyNapEdits, overlap rejectionRead weight/height/sex/DOB from platform health storeAutomatic daily/weekly database backup to Documents folderShared phase engine for breathing and interval timerFour patterns, real timer, per-phase strap buzz, session historyNew interval timer screen using shared breath phase engineAdd/remove nap UI; detected naps suppressed by overlap matchHealth-store import row and automatic backup row in profileWire backup, nap-edit reanalysis, breathing pattern/history/buzz4 files
Tests for nap edit merge, overlap rejection, and validationTests for backup scheduling, filename ordering, and pruningTests for health-store snapshot, merge policy, and permissionsTests for phase engine timing and interval pattern helpers3 files
Add backup cadence and last-run timestamp preference keysAdd READ_WEIGHT and READ_HEIGHT Health Connect permissionsAdd UIFileSharingEnabled and LSSupportsOpeningDocumentsInPlace1 files
Update analytics sibling pin for journal numeric correlations7 files
Summary by CodeRabbit