fix bugs - #172
Conversation
|
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 updates derivation boundaries, unsettled-day analytics, partial-result persistence, briefing prompts, live-step recovery, alarm confirmation, calibration, and TodayScreen live-step updates. ChangesCore behavior updates
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant AppState
participant SharedPreferences
participant LocalStorage
participant TodayScreen
AppState->>SharedPreferences: checkpoint committed live-step data
AppState->>SharedPreferences: recover orphaned checkpoint
AppState->>LocalStorage: record recovered steps
AppState->>TodayScreen: update liveSteps
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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 43d7613)Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Latest suggestions up to 43d7613 Explore these optional code suggestions:
Previous suggestionsSuggestions up to commit bd4c59d
Suggestions up to commit 03a9980
Suggestions up to commit ffea4a0
Suggestions up to commit 487194d
|
|
Went through the bot findings:
|
|
Persistent review updated to latest commit 487194d |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/data/db.dart (1)
2678-2731: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winNarrow the
partialgate to only the metric_series keys the second half actually produces.
partialis set when only the offloaded second-half compute (naps/workouts/HRR/wear/curves/wake-features) failed or timed out. The first-isolate scalars —rhr,rmssd,sdnn,readiness,ln_rmssd,resp_rate,skin_temp_z,skin_temp_adc,dip_pct,trimp,strain_effort,odi_per_hour,cpc_ratio,stress,spo2— are computed before the second half even runs and are never touched by it (see_computeDayBlocksinderivation_engine.dart, which only patches wake features, steps/energy, naps, sleep periods, workouts, wrist orientation, restlessness, and fit quality).Gating the whole
seriesmap on!partialwithholds these already-correct values frommetric_series— the table trend charts andtrailingSeriesValues(the rolling readiness/illness baseline input, seederivation_engine.dart) read from — for as long as the day stays partial. A recurring transient failure (e.g., a consistently slow device) can keep a day partial across multiple passes, creating real gaps in trend history and in the baseline other days' readiness/illness scores are computed against, even though the metrics being dropped were never incomplete.♻️ Proposed fix: only withhold second-half-dependent keys when partial
}, conflictAlgorithm: ConflictAlgorithm.replace); - // A `partial` row already doesn't count as "derived" for the raw-pruning - // guard (see above) — extend the same caution to the rolling baselines: - // don't let a day whose second-half compute failed/timed out overwrite - // (or seed, for a brand-new day) the value tomorrow's readiness/illness - // baseline reads via metric_series. The next successful (non-partial) - // pass writes the real value once it lands. - if (!partial) { - for (final e in series.entries) { + // Second-half-dependent keys only: a `partial` row's headline scalars + // (rhr/rmssd/readiness/etc.) are already correct — only the values the + // offloaded second half produces (naps/workouts/wear/steps/curves) are + // suspect when partial, so only those are withheld. + const secondHalfOnlyKeys = { + 'nap_min', 'hrr_bpm', 'steps', 'active_min', 'strain', 'calories', + 'calories_total', 'worn_min', 'rem_min', 'deep_min', 'light_min', + 'tst_min', 'efficiency', 'lf_hf', 'hrv_cv', 'irregular_rhythm_flag', + 'brv_cv', 'dyn_p90', + }; + for (final e in series.entries) { + if (!partial || !secondHalfOnlyKeys.contains(e.key)) { await txn.insert('metric_series', { 'date': dayId, 'key': e.key, 'value': e.value, }, conflictAlgorithm: ConflictAlgorithm.replace); } }🤖 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 2678 - 2731, Update putDayResult so partial only suppresses metric_series entries produced by the second-half computation, while still persisting first-isolate scalar keys such as rhr, rmssd, readiness, and the other listed baseline metrics. Filter series entries by the second-half-dependent key set before inserting into metric_series; retain normal insertion for all keys when partial is false.
🤖 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 1407-1446: Update _timezoneTravelSuspected so it does not
overwrite the persisted tz_travel_guard baseline when the offset change meets
_tzJumpThresholdMin; retain the prior baseline while returning true, and refresh
it only when no jump is detected. Preserve the existing first-observation
behavior and ensure subsequent derive passes continue reporting the travel
suspicion until the baseline is explicitly refreshed.
- Around line 2353-2364: Update buildCrossDayBundle so today’s unfinalized row
remains available to days/recent and readiness_glassbox processing, while
excluding or replacing only its input to the illness and anomaly CUSUM
calculations. Adjust the shared filtering around _runIsolateCancellable and the
downstream illness/anomaly builders without changing finalized-day behavior.
In `@lib/state/app_state.dart`:
- Around line 2059-2065: Replace the manually constructed day string in the live
coverage block with the existing dayLabelOf() helper, passing the appropriate
window timestamp. Remove the local DateTime and string-building logic while
preserving the existing LocalDb.addLiveCoverage call and its timestamp
arguments.
- Around line 2798-2800: Ensure _recoverOrphanedLiveSession() runs during the
background cold-launch path in _init() after a successful
engine.connectToRemoteId call and before starting backfill work, while
preserving the existing full-connect recovery. Also invoke recovery before
openSession() returns through its fast-reclaim path so orphaned checkpoints are
folded into live_coverage for every launch route.
- Around line 2538-2568: Update _onAlarmGraceElapsed after await
engine.setAlarm(when) to return immediately when _disposed is true or when the
current alarm no longer matches the retry’s when value. Perform both guards
before calling _alarm.set, cancelling or scheduling _alarmGraceTimer, or
notifying listeners, so an in-flight retry cannot recreate timers or overwrite
newer alarm state.
- Around line 1830-1839: Add a raw, uncushioned step-count accessor alongside
liveSteps, preserving the existing _liveRaw gain-and-rounding calculation.
Update _finalizeLivePedometer and finishStepCalibration to use this raw accessor
for LocalDb.addLiveCoverage persistence and ana.calibrateCadence input, while
retaining liveSteps only for display-related reads.
- Around line 2525-2568: Extract the duplicated grace-timer construction into a
helper named _armGraceTimer(DateTime when), preserving the existing duration and
unawaited _onAlarmGraceElapsed(when) callback. Replace the inline Timer blocks
in setAlarm and _onAlarmGraceElapsed with calls to _armGraceTimer(when).
- Around line 1953-1956: Update the cushion assignment in the session-end logic
so a new positive steps value cannot replace an existing active cushion with a
smaller value; retain the larger cushion and its original timestamp, while still
initializing or updating the cushion when the new value is greater or no active
cushion exists.
---
Outside diff comments:
In `@lib/data/db.dart`:
- Around line 2678-2731: Update putDayResult so partial only suppresses
metric_series entries produced by the second-half computation, while still
persisting first-isolate scalar keys such as rhr, rmssd, readiness, and the
other listed baseline metrics. Filter series entries by the
second-half-dependent key set before inserting into metric_series; retain normal
insertion for all keys when partial is false.
🪄 Autofix (Beta)
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: 3584f50b-de2a-48a4-9fa1-8c7600aa9848
📒 Files selected for processing (6)
lib/ai/briefing_engine.dartlib/compute/derivation_engine.dartlib/compute/derive_prepare.dartlib/data/db.dartlib/state/app_state.dartlib/ui/today/today_screen.dart
|
Persistent review updated to latest commit ffea4a0 |
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/compute/derivation_engine.dart (3)
1407-1419: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse
dayLabelOf()for adjacent local-day labels.The local
label()function duplicates local day-label formatting. UsedayLabelOf()for both adjacent dates.Proposed fix
- String label(DateTime x) => - '${x.year.toString().padLeft(4, '0')}-' - '${x.month.toString().padLeft(2, '0')}-' - '${x.day.toString().padLeft(2, '0')}'; return [ - label(DateTime(d.year, d.month, d.day - 1)), - label(DateTime(d.year, d.month, d.day + 1)), + dayLabelOf(DateTime(d.year, d.month, d.day - 1)), + dayLabelOf(DateTime(d.year, d.month, d.day + 1)), ];As per coding guidelines,
lib/**/*.dart: “Use todayLabel() or dayLabelOf() from data/day_label.dart for local day labels; do not derive labels from UTC strings.”🤖 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 1407 - 1419, Update _adjacentDayIds to use the existing dayLabelOf() helper for both DateTime values instead of the local label() formatter, while preserving the current DST-safe adjacent-date calculation and invalid-input behavior.Source: Coding guidelines
3721-3729: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winBump the analytics version for the changed nap attribution.
This changes persisted
sleep_periods,naps, andnap_min. Existing finalized rows remain on the old attribution logic because they are keyed by the unchangedkAlgoVersion. BumpkAlgoVersion, add its changelog entry, and add a regression test for a nap that starts before midnight and ends after midnight.As per coding guidelines,
lib/compute/derivation_engine.dart: “Bump kAlgoVersion and add a changelog entry whenever analytics output changes.”🤖 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 3721 - 3729, The nap attribution change in the derivation flow alters persisted sleep analytics, so update kAlgoVersion in derivation_engine.dart and add the corresponding changelog entry. Add a regression test covering a nap that starts before midnight and ends after midnight, verifying the updated sleep_periods, naps, and nap_min attribution.Source: Coding guidelines
3721-3729: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftUse one nap-segmentation result for both outputs.
_sleepPeriods()and_attachNaps()classify naps with different algorithms. They can produce conflictingsleep_periods,naps, andnap_minvalues for the same buffered substrate. Derive one attributed nap set, then use it for both output blocks.As per coding guidelines,
lib/**/*.dart: “Maintain one source per concern: one raw decode point, sleep segmentation path, readiness path, frame-ingest path, and notification emitter.”🤖 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 3721 - 3729, Update the nap handling around _sleepPeriods and _attachNaps to derive a single attributed nap-segmentation result from inp.napSub, onset, offset, and attributionEndSec, then reuse that result for both bundlePatch['sleep_periods'] and the nap attachment output. Remove the competing classification path so sleep_periods, naps, and nap_min consistently reflect the same attributed nap set.Source: Coding guidelines
lib/state/app_state.dart (1)
2042-2065: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftKeep valid orphan checkpoints until durable recovery succeeds.
Live frames can repeat the same
recTs, soendTs == startTsis valid input forderiveLiveCoverageWindow(). The currentendTs <= startTscheck drops these recoverable sessions before that function can use the ingest-time bounds.The code also removes the checkpoint before
LocalDb.addLiveCoverage()completes. If that write fails, the catch block logs the error and permanently loses the steps. Accept equal timestamps, persist with an idempotent recovery key, and clear the checkpoint only after successful persistence. Add tests for equal timestamps and a failed persistence retry.As per coding guidelines,
test/**/*.dart: “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 2042 - 2065, Update the checkpoint recovery flow around deriveLiveCoverageWindow and LocalDb.addLiveCoverage to allow endTs == startTs, preserving only the invalid endTs < startTs case so ingest-time bounds can recover repeated recTs sessions. Persist recovery idempotently using a stable checkpoint/recovery key, and move removal of _kLiveSessionCheckpoint until after addLiveCoverage completes successfully so failures retain the checkpoint for retry. Add regression tests covering equal timestamps and persistence failure followed by successful retry.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/state/app_state.dart`:
- Around line 2797-2803: Reorder the live-session initialization so
_recoverOrphanedLiveSession() and _resetLivePedometer() complete before
engine.enableLiveStreams() begins delivering frames, or otherwise gate ingestion
until initialization finishes. Add a regression test that injects a live frame
while recovery is pending and verifies the frame is not cleared or
misattributed.
---
Outside diff comments:
In `@lib/compute/derivation_engine.dart`:
- Around line 1407-1419: Update _adjacentDayIds to use the existing dayLabelOf()
helper for both DateTime values instead of the local label() formatter, while
preserving the current DST-safe adjacent-date calculation and invalid-input
behavior.
- Around line 3721-3729: The nap attribution change in the derivation flow
alters persisted sleep analytics, so update kAlgoVersion in
derivation_engine.dart and add the corresponding changelog entry. Add a
regression test covering a nap that starts before midnight and ends after
midnight, verifying the updated sleep_periods, naps, and nap_min attribution.
- Around line 3721-3729: Update the nap handling around _sleepPeriods and
_attachNaps to derive a single attributed nap-segmentation result from
inp.napSub, onset, offset, and attributionEndSec, then reuse that result for
both bundlePatch['sleep_periods'] and the nap attachment output. Remove the
competing classification path so sleep_periods, naps, and nap_min consistently
reflect the same attributed nap set.
In `@lib/state/app_state.dart`:
- Around line 2042-2065: Update the checkpoint recovery flow around
deriveLiveCoverageWindow and LocalDb.addLiveCoverage to allow endTs == startTs,
preserving only the invalid endTs < startTs case so ingest-time bounds can
recover repeated recTs sessions. Persist recovery idempotently using a stable
checkpoint/recovery key, and move removal of _kLiveSessionCheckpoint until after
addLiveCoverage completes successfully so failures retain the checkpoint for
retry. Add regression tests covering equal timestamps and persistence failure
followed by successful retry.
🪄 Autofix (Beta)
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: 1de7ab69-01f7-4eb6-b7e6-2372664cc95f
📒 Files selected for processing (4)
lib/ai/briefing_engine.dartlib/compute/derivation_engine.dartlib/state/app_state.darttest/ai_briefing_test.dart
…lert scoping - liveSteps' display cushion leaked into persistence and calibration: _finalizeLivePedometer and finishStepCalibration read the same getter, so a short session ending inside the prior session's 20s grace window persisted the PRIOR total again (double-counted live_coverage + a bogus cadence). Added _rawSessionSteps and used it wherever the true count is needed. - The cushion could also be lowered by that follow-up session, reintroducing the very regression it exists to prevent — it now never moves down. - Orphan step recovery never ran on a headless cold launch: a process kill plus iOS BLE-restore relaunch takes _init()'s background branch, not openSession(), so checkpointed steps sat in prefs forever. Also moved recovery + reset to BEFORE enableLiveStreams so a frame arriving during the awaited recovery isn't wiped by the reset that followed it. - Alarm auto-retry: guard on _disposed after the await (it was creating a timer dispose() could no longer cancel), bail if a newer alarm was armed while the retry was in flight (it could clobber the fresh one's confirmation), give the retry back when the write itself never landed, and extract the duplicated grace-timer wiring into one helper. - Timezone-travel hold was defeated when EVERY pending day was held: `pending` emptied and fell through to 'latest-finalized-check', which re-derived rawDays.last — one of the days just held. Returns an empty scope now. - Today's unfinalized row is FLAGGED, not dropped, from the cross-day input. Dropping it kept it out of the illness CUSUM but also removed it from readiness/glass-box, the resting-HR trend-shift CUSUM, load, sleep debt and `recent` (whose last row dates every notification). buildCrossDayBundle now nulls only the illness/anomaly/temp inputs for a flagged day. - Day labels go through dayLabelOf() at both live-coverage write sites. Tests 1055 -> 1058, analyze clean.
|
Went through the remaining bot findings against current Fixed
Already fixed on the branch, no action
Declined
|
|
Persistent review updated to latest commit 03a9980 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/state/app_state.dart (1)
1961-1994: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winClear the checkpoint before, not after, the durable write — avoid double-counting on a kill.
_finalizeLivePedometerwrites toLocalDb.addLiveCoverageand only clears_kLiveSessionCheckpointafterward (Line 1985-1994). If the process is killed between theaddLiveCoveragewrite completing and_clearLiveSessionCheckpointfinishing, the stale checkpoint survives. On the next launch,_recoverOrphanedLiveSession(Line 2059-2090) reads that same checkpoint and callsaddLiveCoverageagain with the same window and step count — double-counting those steps into the day's total and into the cadence-calibration input.
_recoverOrphanedLiveSessionalready uses the safer order (clears the checkpoint, Line 2064, before callingaddLiveCoverage, Line 2085) — a kill in that window loses a few steps rather than duplicating them. Make_finalizeLivePedometerconsistent with that ordering so a kill in either function fails toward "lose a little" rather than "double-count."🐛 Proposed fix
_resetLivePedometer(); + // The session ended cleanly — drop the checkpoint BEFORE writing + // live_coverage. A kill in this narrow window then loses this bout's + // steps rather than double-counting them on the next recovery pass. + await _clearLiveSessionCheckpoint(); // Record the REAL 100 Hz step window (device time). The derivation pass adds // it to the day's steps AND excludes those minutes from the 1 Hz estimate, so // 100 Hz always wins and a minute is never counted twice. if (window != null) { final day = dayLabelOf( DateTime.fromMillisecondsSinceEpoch(window.startTs * 1000), ); await LocalDb.addLiveCoverage(window.startTs, window.endTs, steps, day); } - // The session ended cleanly and is now durably recorded — the checkpoint - // that would otherwise let a killed-process session recover is no longer - // needed. - await _clearLiveSessionCheckpoint(); if (steps <= 0 || durS < 20) return;🤖 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 1961 - 1994, Update _finalizeLivePedometer to await _clearLiveSessionCheckpoint immediately before the durable LocalDb.addLiveCoverage call, while preserving the existing window calculation and reset flow. Remove the later checkpoint-clear call so recovery cannot replay an already-written coverage window.lib/compute/derivation_engine.dart (1)
1687-1735: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPass the buffered nap slice through CSV/WHOOP import days.
PreparedDerivationDay.napSubdefaults todaySub, butderive_prepare.dartusessub.slice(day.startSec, day.endSec + napBoundaryBufferSec)for nap detection.lib/compute/derivation_engine.dart:1718passesSubstrate.emptyfromday napSub, so imported days cannot span midnight naps into the next calendar day. Add the bufferednapSubslice here or through the importedPhysioDay/DerivationDaypath.🤖 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 1687 - 1735, Update _deriveDay to populate PreparedDerivationDay.napSub with a substrate slice extending day.endSec by napBoundaryBufferSec, matching the buffered range used by derive_prepare.dart. Ensure CSV/WHOOP-imported days pass this buffered nap data through instead of the default or Substrate.empty, while preserving the existing daySub and sleepSub behavior.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.
Outside diff comments:
In `@lib/compute/derivation_engine.dart`:
- Around line 1687-1735: Update _deriveDay to populate
PreparedDerivationDay.napSub with a substrate slice extending day.endSec by
napBoundaryBufferSec, matching the buffered range used by derive_prepare.dart.
Ensure CSV/WHOOP-imported days pass this buffered nap data through instead of
the default or Substrate.empty, while preserving the existing daySub and
sleepSub behavior.
In `@lib/state/app_state.dart`:
- Around line 1961-1994: Update _finalizeLivePedometer to await
_clearLiveSessionCheckpoint immediately before the durable
LocalDb.addLiveCoverage call, while preserving the existing window calculation
and reset flow. Remove the later checkpoint-clear call so recovery cannot replay
an already-written coverage window.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 65806142-5934-450c-ad42-c26816178f07
📒 Files selected for processing (4)
lib/compute/crossday_pipeline.dartlib/compute/derivation_engine.dartlib/state/app_state.darttest/crossday_pipeline_test.dart
…replay-safe recovery - _deriveDay (the CSV/WHOOP import path) never passed napSub, so it fell back to daySub and bisected midnight-straddling naps again — on exactly the path this PR set out to fix. Passes the same buffered slice prepareDerivationPayload uses. - The tz-travel guard's baseline was re-written in _deriveScope's force branch, i.e. at scope-SELECTION time, before the restage had actually run. An interrupted restage therefore dropped the hold anyway. Moved to after the full-history pass completes. - Orphan step recovery is now replay-safe. _finalizeLivePedometer writes live_coverage before clearing the checkpoint (durable-first, deliberately); a kill in that gap left a checkpoint whose bout was already banked, and live_coverage has no uniqueness on the window, so recovery would inflate the day's real steps. Added LocalDb.hasLiveCoverageWindow and skip on a hit — keeps the safe write ordering instead of trading a double-count for a loss. Tests 1058 passing, analyze clean.
|
Second round of bot findings, checked against current code — three were real and are fixed in cc126ee. Fixed
Declined
|
|
Persistent review updated to latest commit cc126ee |
PR Code Suggestions ✨No code suggestions found for the PR. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (10)
lib/compute/derivation_engine.dart (3)
1422-1434: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse
dayLabelOf()for adjacent local labels.
_adjacentDayIdsperforms safe date arithmetic, but itslabelfunction creates a second local day-label implementation. UsedayLabelOf(DateTime(...))for both adjacent dates.As per coding guidelines,
lib/**/*.dartmust usetodayLabel()ordayLabelOf()for local day labels.🤖 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 1422 - 1434, Update _adjacentDayIds to use the existing dayLabelOf function for both DateTime values instead of defining the local label formatter. Preserve the current DST-safe date arithmetic and empty-list behavior when dayId cannot be parsed.Source: Coding guidelines
1985-1989: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftEmit one notification for each eligible workout bout.
The dedupe key is per bout, but
_computeWorkoutsstill returns only the newestnotifBout._derivePreparedDayemits one event. If two bouts end within the two-hour window, only the newest bout generates a notification.Return all eligible bout descriptors and call the existing notification emitter once per bout. Keep
${day.date}:auto_workout:${bout.endSec}as the per-bout dedupe key.🤖 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 1985 - 1989, Update _computeWorkouts to return all eligible workout-bout descriptors instead of only the newest notifBout, then update _derivePreparedDay to invoke the existing notification emitter once for each returned bout. Preserve eligibility filtering and use `${day.date}:auto_workout:${bout.endSec}` for each bout’s dedupe key.
3394-3399: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueDocument the nap attribution change in the v51 release notes.
kAlgoVersionis already51, so this version bump is present. Add an explicit derivation changelog entry for thesleep_periods,naps, andnap_minoutput changes so the bumped version matches the changed analytics output.🤖 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 3394 - 3399, Update the v51 release notes or derivation changelog to explicitly document the nap attribution changes affecting the sleep_periods, naps, and nap_min outputs, ensuring the entry corresponds to the existing kAlgoVersion 51 bump.Source: Coding guidelines
lib/state/app_state.dart (7)
2067-2074: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAllow equal band timestamps during orphan recovery.
Live frames commonly repeat the same
recTs, sostartTs == endTsis valid.deriveLiveCoverageWindow()reconstructs duration from the phone-clock ingest bounds.The
endTs <= startTsguard returns before that helper runs. Normal checkpoints are therefore discarded, and their steps are never banked. Reject onlyendTs < startTs.Minimal fix
- if (steps <= 0 || startTs == null || endTs == null || endTs <= startTs) { + if (steps <= 0 || startTs == null || endTs == null || endTs < startTs) {Add a regression test with repeated
recTsvalues.🤖 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 2067 - 2074, Update the validation guard in the orphan-recovery flow around deriveLiveCoverageWindow to reject only endTs < startTs, allowing equal timestamps to reach the helper and bank their steps. Add a regression test covering repeated recTs values and confirming the resulting steps are preserved.
1922-1925: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftSerialize checkpoint writes with session finalization.
_checkpointLiveSession()reads session fields afterawait SharedPreferences.getInstance(). A disconnect can reset those fields before the continuation runs. The checkpoint can then contain data from a different session.The reverse race can also occur.
_finalizeLivePedometer()can clear a newer session's checkpoint afterawait LocalDb.addLiveCoverage()completes.Capture an immutable snapshot before the first await. Associate it with a session generation. Serialize checkpoint writes and finalization. Clear only the checkpoint for the finalized generation.
Also applies to: 1991-1994, 2025-2039
🤖 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 1922 - 1925, Serialize _checkpointLiveSession() and _finalizeLivePedometer() so checkpoint writes and finalization cannot overlap. Capture all session fields needed by _checkpointLiveSession before its first await, associate the snapshot and checkpoint with a monotonically tracked session generation, and ignore stale continuations. During finalization, clear only the checkpoint belonging to the finalized generation, preserving newer session data.
2061-2064: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftKeep the checkpoint until coverage is durable.
For a valid checkpoint,
_recoverOrphanedLiveSession()removes the preference beforeLocalDb.addLiveCoverage(). A process kill or database failure in that gap loses the only recovery record.The deduplication check is also not atomic. Concurrent recovery or finalization can both observe no row and insert duplicate coverage because
live_coveragehas no unique window constraint.Remove the checkpoint only after a successful insert or a confirmed existing row. Make the check-and-insert atomic with a database transaction or a unique idempotency key.
Also applies to: 2082-2094
🤖 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 2061 - 2064, Update _recoverOrphanedLiveSession() to retain _kLiveSessionCheckpoint until LocalDb.addLiveCoverage() succeeds or an existing matching coverage row is confirmed. Make the deduplication check and insert atomic using a database transaction or unique idempotency key, ensuring concurrent recovery/finalization cannot create duplicates; remove the checkpoint only after that durable outcome.
2535-2559: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftInvalidate stale alarm operations before a new write.
setAlarm()does not cancel or invalidate the previous grace timer before awaitingengine.setAlarm(when). If the old timer expires during a slow new write,_onAlarmGraceElapsed()can send the old alarm time to the strap.The post-await
_savedAlarmcheck prevents stale local state updates. It cannot undo a stale hardware write. Serialize alarm operations or use an operation generation. Invalidate the previous generation before awaiting a new write, and verify the generation before retrying or applying the result.Also applies to: 2577-2604
🤖 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 2535 - 2559, Update setAlarm and the related _onAlarmGraceElapsed retry flow to invalidate the previous alarm operation before awaiting engine.setAlarm(when), using an operation generation or equivalent serialization. Ensure the grace timer and any retry verify the current generation before writing to the strap, and verify it again before applying post-await state or persistence so stale operations cannot affect hardware or local alarm state.
2542-2560: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard
setAlarm()continuations after disposal.
setAlarm()can resume afterdispose()fromengine.setAlarm()orSharedPreferences. It can then mutate alarm state and create_alarmGraceTimerafterdispose()has canceled owned timers.The
notifyListeners()guard does not prevent the new timer from being created. Check_disposedafter each await and before arming the timer.🤖 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 2542 - 2560, Guard the asynchronous continuation in the alarm-setting method after each await, including engine.setAlarm and SharedPreferences.getInstance/setInt, by returning when _disposed is true before mutating alarm state or persisting values. Also check _disposed immediately before _armAlarmGraceTimer so disposal cannot create a new timer, while preserving the existing failure handling and notification behavior.
2838-2844: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftComplete checkpoint handling on every live-session path.
openSession()now recovers before resetting counters._reconnect()at Lines 2946-2956 still enables live streams and calls_resetLivePedometer()without recovery. A checkpoint left by a failed finalization can be overwritten by the next session.
finishStepCalibration()andcancelStepCalibration()also reset the shared counters without clearing or finalizing the checkpoint. Calibration data can later be replayed as orphanedlive_coverage, or be overwritten by a later checkpoint.Add recovery before every new-session reset. Add explicit finalize-or-clear behavior for calibration, including pending checkpoint writes.
As per coding guidelines,
lib/**/*.dart: “When adding or changing a capability, cover every call path, including all raw decode paths and all relevant export/session triggers.”Also applies to: 3421-3421
🤖 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 2838 - 2844, Complete checkpoint handling across all live-session entry paths: update _reconnect() to recover orphaned sessions before enabling streams or calling _resetLivePedometer(). Update finishStepCalibration() and cancelStepCalibration() so calibration resets explicitly finalize or clear the checkpoint, including awaiting any pending checkpoint writes. Preserve the existing openSession() ordering and ensure no session path can overwrite an unresolved checkpoint.Source: Coding guidelines
2032-2038: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftKeep the recovered window aligned with committed steps.
The checkpoint stores committed
steps, but it stores total_liveSamplesand the latest ingest bounds. Those fields include the uncommitted partial minute.
_recoverOrphanedLiveSession()passes the mixed values toderiveLiveCoverageWindow(). The resulting coverage row can exclude the partial minute from the 1 Hz estimator without adding its steps.Store the committed sample and time frontier, or persist enough state to recover the partial minute.
Also applies to: 2073-2079
🤖 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 2032 - 2038, Align checkpointed coverage data with the committed steps in the live-session state serialization. Update the fields written near `steps` and the corresponding handling in `_recoverOrphanedLiveSession()` so `deriveLiveCoverageWindow()` receives committed samples and time frontiers, or enough partial-minute state to restore them consistently; do not pass total `_liveSamples` or latest ingest bounds when they include uncommitted data.
🤖 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 881-888: Track timezone-travel hold state explicitly instead of
clearing it when the current offset matches the baseline. Update
_timezoneTravelSuspected to preserve the hold until a successful full restage
completes, despite skipped or failed processDay calls. Ensure both the current
full-restage flow and runDays(force: true) reset the hold only after every
targeted day succeeds; interrupted or partially failed restages must leave it
held.
- Around line 1700-1703: Update _prepareTargetDay to load a buffered nap
substrate spanning day.startSec through day.endSec + napBoundaryBufferSec, and
add an explicit napSub field and constructor parameter to PreparedDerivationDay.
Pass this napSub through _derivePreparedDay into candidate.toPreparedDay instead
of relying on the daySub fallback, while keeping import-data verification
separate and requiring callers to provide post-day-end samples.
In `@lib/data/db.dart`:
- Around line 793-808: Make live-coverage recovery atomic by replacing the
separate hasLiveCoverageWindow check and addLiveCoverage call with a
transactionally deduplicated insert for the same start/end window. Update the
recovery caller to use the insert result and only treat the checkpoint as newly
recovered when the insert succeeds; preserve liveStepsForDay’s existing
aggregation behavior.
---
Outside diff comments:
In `@lib/compute/derivation_engine.dart`:
- Around line 1422-1434: Update _adjacentDayIds to use the existing dayLabelOf
function for both DateTime values instead of defining the local label formatter.
Preserve the current DST-safe date arithmetic and empty-list behavior when dayId
cannot be parsed.
- Around line 1985-1989: Update _computeWorkouts to return all eligible
workout-bout descriptors instead of only the newest notifBout, then update
_derivePreparedDay to invoke the existing notification emitter once for each
returned bout. Preserve eligibility filtering and use
`${day.date}:auto_workout:${bout.endSec}` for each bout’s dedupe key.
- Around line 3394-3399: Update the v51 release notes or derivation changelog to
explicitly document the nap attribution changes affecting the sleep_periods,
naps, and nap_min outputs, ensuring the entry corresponds to the existing
kAlgoVersion 51 bump.
In `@lib/state/app_state.dart`:
- Around line 2067-2074: Update the validation guard in the orphan-recovery flow
around deriveLiveCoverageWindow to reject only endTs < startTs, allowing equal
timestamps to reach the helper and bank their steps. Add a regression test
covering repeated recTs values and confirming the resulting steps are preserved.
- Around line 1922-1925: Serialize _checkpointLiveSession() and
_finalizeLivePedometer() so checkpoint writes and finalization cannot overlap.
Capture all session fields needed by _checkpointLiveSession before its first
await, associate the snapshot and checkpoint with a monotonically tracked
session generation, and ignore stale continuations. During finalization, clear
only the checkpoint belonging to the finalized generation, preserving newer
session data.
- Around line 2061-2064: Update _recoverOrphanedLiveSession() to retain
_kLiveSessionCheckpoint until LocalDb.addLiveCoverage() succeeds or an existing
matching coverage row is confirmed. Make the deduplication check and insert
atomic using a database transaction or unique idempotency key, ensuring
concurrent recovery/finalization cannot create duplicates; remove the checkpoint
only after that durable outcome.
- Around line 2535-2559: Update setAlarm and the related _onAlarmGraceElapsed
retry flow to invalidate the previous alarm operation before awaiting
engine.setAlarm(when), using an operation generation or equivalent
serialization. Ensure the grace timer and any retry verify the current
generation before writing to the strap, and verify it again before applying
post-await state or persistence so stale operations cannot affect hardware or
local alarm state.
- Around line 2542-2560: Guard the asynchronous continuation in the
alarm-setting method after each await, including engine.setAlarm and
SharedPreferences.getInstance/setInt, by returning when _disposed is true before
mutating alarm state or persisting values. Also check _disposed immediately
before _armAlarmGraceTimer so disposal cannot create a new timer, while
preserving the existing failure handling and notification behavior.
- Around line 2838-2844: Complete checkpoint handling across all live-session
entry paths: update _reconnect() to recover orphaned sessions before enabling
streams or calling _resetLivePedometer(). Update finishStepCalibration() and
cancelStepCalibration() so calibration resets explicitly finalize or clear the
checkpoint, including awaiting any pending checkpoint writes. Preserve the
existing openSession() ordering and ensure no session path can overwrite an
unresolved checkpoint.
- Around line 2032-2038: Align checkpointed coverage data with the committed
steps in the live-session state serialization. Update the fields written near
`steps` and the corresponding handling in `_recoverOrphanedLiveSession()` so
`deriveLiveCoverageWindow()` receives committed samples and time frontiers, or
enough partial-minute state to restore them consistently; do not pass total
`_liveSamples` or latest ingest bounds when they include uncommitted data.
🪄 Autofix (Beta)
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: 475617c1-7852-4335-b10a-70d40fe085cf
📒 Files selected for processing (3)
lib/compute/derivation_engine.dartlib/data/db.dartlib/state/app_state.dart
| /// True when a coverage row for exactly this window already exists. | ||
| /// | ||
| /// `live_coverage` is an append-only SUM (no uniqueness on the window), so a | ||
| /// replayed write double-counts the day's real steps. The orphaned-session | ||
| /// recovery uses this to stay idempotent: a process killed AFTER | ||
| /// `_finalizeLivePedometer` wrote coverage but BEFORE it cleared the | ||
| /// checkpoint would otherwise re-add the same bout on the next launch. | ||
| static Future<bool> hasLiveCoverageWindow(int startTs, int endTs) async { | ||
| final db = await instance; | ||
| final r = await db.rawQuery( | ||
| 'SELECT 1 FROM live_coverage WHERE start_ts = ? AND end_ts = ? LIMIT 1', | ||
| [startTs, endTs], | ||
| ); | ||
| return r.isNotEmpty; | ||
| } | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make live-coverage recovery atomic.
hasLiveCoverageWindow only reads. The caller checks it and then calls addLiveCoverage in a separate operation. live_coverage has no unique constraint on (start_ts, end_ts). Two recovery paths can both observe no row and insert the same checkpoint. liveStepsForDay then counts the steps twice.
Combine the check and insert in one transaction, or add a unique window key with a deduplicating migration. Make recovery use the atomic insert result.
🤖 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 793 - 808, Make live-coverage recovery atomic
by replacing the separate hasLiveCoverageWindow check and addLiveCoverage call
with a transactionally deduplicated insert for the same start/end window. Update
the recovery caller to use the insert result and only treat the checkpoint as
newly recovered when the insert succeeds; preserve liveStepsForDay’s existing
aggregation behavior.
…lure, single-flight recovery - The napSub fix only ever reached the import path. _prepareTargetDay (used by run()/runDays()/rescanRecent() — i.e. EVERY non-imported day) still called toPreparedDay without napSub, so it fell back to daySub and kept bisecting cross-midnight naps. It now loads [dayStart, dayEnd + napBoundaryBufferSec) in ONE substrate pass (each load spawns its own isolate, so a second load would double the cost) and slices the calendar day back out of it. - The tz-travel guard was re-baselined after ANY full restage, but processDay swallows per-day errors and marks the day skipped — a restage could 'finish' with days still unresolved and drop the hold anyway. Only clears now when every targeted day actually derived. Not applied to runDays(): that is the selected-days path, not the full-restage entry point (reanalyzeAll calls run(force: true)). - Orphan step recovery is single-flight. Two entry points can call it now, and interleaving would let both read the checkpoint before either removed it; live_coverage is an append-only SUM with no window uniqueness, so the duplicate would inflate the day. Tests 1058 passing, analyze clean.
|
Third round — all three CodeRabbit findings on cc126ee were real. Fixed in bd4c59d. PR Agent reported "No code suggestions found" for this commit. Fixed
Partially declined
|
|
Persistent review updated to latest commit bd4c59d |
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 (3)
lib/state/app_state.dart (2)
2071-2106: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPersist recovered coverage before clearing the checkpoint.
Line 2076 removes the checkpoint before validation and before
LocalDb.addLiveCoverage. Repeated live-framerecTsvalues makeendTs == startTsnormal, but Lines 2080-2083 reject that checkpoint beforederiveLiveCoverageWindowcan use the ingest-time bounds. A database failure after removal also loses the recovery data permanently.Derive the coverage window first. Accept equal band timestamps when the helper derives a valid window. Remove the checkpoint only after existing coverage is detected or the new coverage write succeeds.
🤖 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 2071 - 2106, The _recoverOrphanedLiveSessionOnce method clears _kLiveSessionCheckpoint too early and rejects equal band timestamps before deriveLiveCoverageWindow can recover using ingest bounds. Derive the window first, allow startTs == endTs when the helper returns a valid window, then remove the checkpoint only after existing coverage is detected or LocalDb.addLiveCoverage succeeds; retain the checkpoint if validation fails or persistence errors.
2850-2856: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winApply the frame-ingest ordering to reconnects.
This path recovers and resets before
enableLiveStreams, but_reconnect()still enables streams before calling_resetLivePedometer. A frame received during that await is added to the new session and then erased.Reset the pedometer before enabling either live-stream mode in
_reconnect().As per coding guidelines,
lib/**/*.dart: “When adding or changing a capability, cover every call path, including all raw decode paths and all relevant export/session triggers.”🤖 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 2850 - 2856, Update _reconnect() so _resetLivePedometer() runs before enabling live streams, matching the ordering in the shown recovery path. Ensure both reconnect stream-enable modes reset first, preventing frames received during the awaited enableLiveStreams() call from being erased.Source: Coding guidelines
lib/compute/derivation_engine.dart (1)
3770-3778: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPrevent cross-midnight naps from being attributed twice.
The forward-only buffer lets the prior day detect a nap that crosses midnight, but the next day sees the suffix as a new nap. Filter detected runs by both calendar boundaries after providing prior-boundary context.
lib/compute/derivation_engine.dart#L3770-L3778: pass a lower and upper attribution boundary. Keep only naps whose start is in[dayStartSec, dayEndSec).lib/compute/derive_prepare.dart#L313-L313: include the preceding nap buffer as well as the following buffer.lib/compute/derivation_engine.dart#L1039-L1051: load the preceding nap buffer for live derivation.lib/compute/derivation_engine.dart#L1721-L1725: include the preceding nap buffer for imported derivation.As per coding guidelines,
lib/**/*.dart: “When adding or changing a capability, cover every call path, including all raw decode paths and all relevant export/session triggers.”🤖 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 3770 - 3778, Update the sleep attribution flow so detected naps are restricted to starts in [dayStartSec, dayEndSec): at lib/compute/derivation_engine.dart lines 3770-3778, pass both lower and upper attribution boundaries to _sleepPeriods and _attachNaps. At lib/compute/derive_prepare.dart lines 313-313, include the preceding nap buffer alongside the following buffer. At lib/compute/derivation_engine.dart lines 1039-1051 and 1721-1725, load/include the preceding nap buffer for live and imported derivation paths respectively.Source: Coding guidelines
♻️ Duplicate comments (1)
lib/state/app_state.dart (1)
2589-2619: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not let a stale retry consume the newer alarm retry.
A retry for alarm A sets
_alarmAutoRetried = truebefore awaitingengine.setAlarm. If the user sets alarm B during that await,setAlarmresets the flag for B. When A then succeeds, this method leaves the flag true because its stale check fails at Line 2614. Alarm B then skips its one automatic retry.After the await, return when
_savedAlarm != epochbefore changing_alarmAutoRetried.🤖 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 2589 - 2619, Update _onAlarmGraceElapsed so that after awaiting engine.setAlarm(when), it checks whether _savedAlarm still equals epoch before modifying _alarmAutoRetried. Return immediately for a stale retry, preserving the newer alarm’s retry state; keep the existing dispose and rearm handling for the current alarm unchanged.
🤖 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 884-899: Update runDays to track unresolved target days and reset
the tz_travel_guard baseline only after a forced selected restage whose selected
set covers the full raw history and has no unresolved days. Ensure
AppState.reanalyzeDays() via runDays(..., force: true) clears the hold on
complete success, while partial selections or failed/skipped targets preserve
the existing baseline.
---
Outside diff comments:
In `@lib/compute/derivation_engine.dart`:
- Around line 3770-3778: Update the sleep attribution flow so detected naps are
restricted to starts in [dayStartSec, dayEndSec): at
lib/compute/derivation_engine.dart lines 3770-3778, pass both lower and upper
attribution boundaries to _sleepPeriods and _attachNaps. At
lib/compute/derive_prepare.dart lines 313-313, include the preceding nap buffer
alongside the following buffer. At lib/compute/derivation_engine.dart lines
1039-1051 and 1721-1725, load/include the preceding nap buffer for live and
imported derivation paths respectively.
In `@lib/state/app_state.dart`:
- Around line 2071-2106: The _recoverOrphanedLiveSessionOnce method clears
_kLiveSessionCheckpoint too early and rejects equal band timestamps before
deriveLiveCoverageWindow can recover using ingest bounds. Derive the window
first, allow startTs == endTs when the helper returns a valid window, then
remove the checkpoint only after existing coverage is detected or
LocalDb.addLiveCoverage succeeds; retain the checkpoint if validation fails or
persistence errors.
- Around line 2850-2856: Update _reconnect() so _resetLivePedometer() runs
before enabling live streams, matching the ordering in the shown recovery path.
Ensure both reconnect stream-enable modes reset first, preventing frames
received during the awaited enableLiveStreams() call from being erased.
---
Duplicate comments:
In `@lib/state/app_state.dart`:
- Around line 2589-2619: Update _onAlarmGraceElapsed so that after awaiting
engine.setAlarm(when), it checks whether _savedAlarm still equals epoch before
modifying _alarmAutoRetried. Return immediately for a stale retry, preserving
the newer alarm’s retry state; keep the existing dispose and rearm handling for
the current alarm unchanged.
🪄 Autofix (Beta)
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: 998167d9-dee4-4357-aec7-06109208463f
📒 Files selected for processing (3)
lib/compute/derivation_engine.dartlib/compute/derive_prepare.dartlib/state/app_state.dart
…tz hold, durable-first recovery - runDays(force: true) — the Advanced > select-days re-analyze path — now also clears the timezone hold, but only when the selection actually covers the whole raw history AND every target resolved. A partial selection still says nothing about the days being held, so it deliberately leaves the hold up. - Orphan step recovery is durable-first again (write coverage, then drop the checkpoint) — the same rule as commit-before-ACK. Removing the checkpoint first meant a kill in that gap lost the bout, which is what the checkpoint exists to prevent. Replay is safe now: the coverage-window check makes it idempotent and the method is single-flight. A checkpoint that can never be recovered (malformed, or a window the sanitizer rejects) is still dropped immediately so it can't be retried on every connect forever. Tests 1058 passing, analyze clean.
|
Fourth round on bd4c59d — two real, two declined. Fixed in 43d7613. Fixed
Declined
|
|
Persistent review updated to latest commit 43d7613 |
|
Fifth round on 43d7613: CodeRabbit posted no new findings (status: "Review completed"). PR Agent raised one, which I'm declining as factually incorrect. Declined — "Non-idempotent timezone guard baseline update" The claim is that "if a timezone jump happens between two normal derive runs, the second normal run will see That is not what the code does. final jumped = (nowOffsetMin - lastOffsetMin).abs() >= _tzJumpThresholdMin;
if (!jumped) {
await LocalDb.putBaseline('tz_travel_guard', ...);
}
return jumped;When a jump is detected the stored offset is deliberately left alone, so every subsequent pass compares against the same pre-travel baseline and keeps returning true until a full restage clears it. That was exactly the fix applied in 487194d, and the suggestion would re-open the hole it closed — with the non-jump write removed, an ordinary DST shift (60 min, well under the 180 min threshold) would leave the baseline permanently stale, and a later legitimate 2 h move measured against a months-old offset would read as travel and hold days for no reason. The one genuine residual is sub-threshold staircase drift (two separate <3 h offset changes summing to more than 3 h are each absorbed). The proposed change doesn't address that either, and a single discrete offset change is what actual travel looks like. Leaving as-is. Merging — CI green, |
1 participant
User description
PR Type
Bug fix
Description
Fix naps straddling midnight being silently dropped by introducing a
napSubbuffer windowFix live step count freezing/dropping on disconnect with a grace cushion and orphan-session recovery
Fix false illness alerts, stale morning briefings, and partial-derive baseline pollution
Fix alarm confirmation retry, per-bout workout notifications, and timezone-travel derive guard
Diagram Walkthrough
File Walkthrough
derive_prepare.dart
Add napSub buffer slice to fix midnight-straddling nap bisectionlib/compute/derive_prepare.dart
napSubfield toPreparedDerivationDay: a substrate slice extended3 hours past the calendar day end so naps straddling midnight are seen
whole
napBoundaryBufferSecconstant (3 h) documenting the buffernapSubintoJson/fromJson; falls back todaySubfor old rowsnapSub = sub.slice(day.startSec, day.endSec +napBoundaryBufferSec)inprepareDerivationPayloadderivation_engine.dart
Fix nap double-count, TZ guard, illness alert, and per-bout workoutnotifylib/compute/derivation_engine.dart
napSubthrough_DayBlocksInputand uses it (withattributionEndSec) in_sleepPeriodsand_attachNapsso naps startingin the buffer but belonging to tomorrow are excluded
_timezoneTravelSuspected()(>=3 h offset jump) and holds pendingdays adjacent to finalized ones until a full restage; resets guard on
force-restage
day_resultrow from the cross-dayillness/anomaly CUSUM feed
dedupeKeyfromdate:auto_workouttodate:auto_workout:endSecso a second real workout the same day is notsilently swallowed
_adjacentDayIdshelper uses DST-safeDateTimearithmeticdb.dart
Skip metric_series writes for partial derive passeslib/data/db.dart
metric_seriesinserts inupsertDayResultwithif (!partial)guard so a failed/timed-out second-half derive pass cannot overwrite
or seed baseline series values
app_state.dart
Fix step freeze, step checkpoint, briefing cache, and alarm auto-retrylib/state/app_state.dart
_sessionStepsCushion/_sessionCushionSetAtMs(20 s grace) toliveStepsso the displayed count never regresses on disconnect beforederivation catches up
_checkpointLiveSession(writes committed steps to SharedPrefsonce per minute) and
_recoverOrphanedLiveSession(replays checkpointon next connect) to survive process kills
overnight_state == 'ready'toavoid caching a briefing off a still-syncing overnight
_alarmAutoRetriedflag and_onAlarmGraceElapsedto silentlyre-arm the alarm once before surfacing the unconfirmed warning
liveStepsto thecontext.selecttuple inTodayScreenso the stepstile updates during a live walk
_recoverOrphanedLiveSession()before_resetLivePedometer()onconnect; clears checkpoint after clean session end
briefing_engine.dart
Remove stale time-of-day greeting from cached briefing promptlib/ai/briefing_engine.dart
instructs the model to start with substance so a cached briefing read
hours later doesn't open with a stale greeting
partOfDaydoc comment to clarify it is generation-time contextonly, not a greeting directive
today_screen.dart
Include liveSteps in Today screen select to unfreeze step tilelib/ui/today/today_screen.dart
a.liveStepsto thecontext.selecttuple so the Today screenrebuilds on live step changes (~1/s) without waiting for an unrelated
dbCountschangeSummary by CodeRabbit
New Features
Bug Fixes