Repository navigation
fix(recovery): provisional/final states, no early pins, one headline source (refs #543) - #578
Open
abdulsaheel wants to merge 7 commits into
Open
abdulsaheel wants to merge 7 commits into
abdulsaheel wants to merge 7 commits into
Conversation
…e source Freeze only final nights (wake before 03:00 or under 3h of sleep never pin), trust a pin only for the night whose wake it matches, and route Home, Coach get_today, the morning briefing and the low-readiness notification through todayHeadlineOf so they cannot disagree. Home notes a changed final number (Updated 8:12 · 2 → 28); re-analysis lists the days whose recovery moved.
# Conflicts: # lib/compute/derivation_engine.dart # lib/l10n/app_en.arb # test/low_readiness_notification_test.dart
Readiness detail greys a provisional number and shows the update note; Health names today's recovery state above the overnight rows (one shared recoveryStateLine). A manual re-analyse releases today's pin so the ring re-pins to the re-derived value. Widgets draw a provisional night in the muted calibrating state; Watch/Siri keep -1 until final. run_sql shadows v_daily/v_metric with temp views whose today readiness is the final headline or NULL. Russian strings for the new keys; the low-readiness agreement fixture pins on the night's own wake.
Reviewer's GuideCentralizes today’s recovery headline around explicit in-progress, provisional, and final states; prevents premature or stale pins; and routes Home, Coach, Readiness, Health, briefing, notifications, and widgets through the same value and state, with UI change notices and broad regression coverage. State diagram for today’s recovery headlinestateDiagram-v2
[*] --> night_in_progress
night_in_progress --> provisional: wake confirmed
provisional --> finalReady: overnightSettled
night_in_progress --> finalReady: overnightSettled
finalReady --> finalReady: final value served
state night_in_progress {
[*] --> no_number
no_number: No recovery number
no_number: Sleeping…
}
state provisional {
[*] --> muted_number
muted_number: Today's number
muted_number: Finishing up
}
state finalReady {
[*] --> frozen_number
frozen_number: Colour number
frozen_number: Eligible for pin
}
Flow diagram for the shared recovery headlineflowchart LR
derived["Derived today data"] --> headline[todayHeadlineOf]
headline --> home[Home]
headline --> readiness[Readiness detail]
headline --> health[Health]
headline --> briefing[Briefing]
headline --> widgets[Widgets]
headline --> coach[get_today]
headline --> views["v_daily / v_metric today"]
views --> coachSql[Coach run_sql]
coachSql --> coach
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 7 minutes. View limit detailsLimit details: You’ve used all 4 included reviews currently available. Review configuration: ⚙️ Run configuration
⛔ Files ignored due to path filters (8)
📒 Files selected for processing (16)
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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 7 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="lib/coach/coach_engine.dart" line_range="805" />
<code_context>
+ readiness: h['recovery_state'] == 'final' ? h['recovery'] as num? : null,
+ );
+ } catch (_) {
+ return null;
+ }
+ }
</code_context>
<issue_to_address>
**Coach exposes partial readiness**
When `api.getToday()` throws while Coach handles a `run_sql` request, `_todayReadiness` returns null, and `_serveToday` treats null as no override, leaving the stored views exposed. Coach SQL can return provisional readiness despite the final-only contract.
Represent a failed lookup separately from a no-override result and mask today’s readiness in the temporary views when the lookup fails.
Also at `lib/coach/coach_db.dart:492`, `lib/coach/coach_db.dart:525`.
</issue_to_address>
### Comment 2
<location path="lib/coach/coach_db.dart" line_range="502-505" />
<code_context>
+ : '"${r['name']}"',
+ ];
+ await db.execute(
+ 'CREATE TEMP VIEW v_daily AS SELECT ${cols.join(', ')} FROM main.v_daily');
+ await db.execute('CREATE TEMP VIEW v_metric AS SELECT date, key, '
+ "CASE WHEN date = '$day' AND key = 'readiness' THEN $v ELSE value END "
+ 'AS value FROM main.v_metric');
+ }
+
</code_context>
<issue_to_address>
**Pinned recovery disappears from Coach**
When a final headline pin exists but today’s readiness row is absent from the underlying views, `_serveToday` only rewrites rows returned by `main.v_daily` and `main.v_metric`; it does not add the missing row. Coach SQL omits the recovery that Home and `get_today` show.
Add today’s pinned readiness row to the temporary views when the underlying row is absent.
</issue_to_address>
### Comment 3
<location path="lib/compute/derivation_engine.dart" line_range="2176-2180" />
<code_context>
}) {
+ // A pin with no wake (written by an older build) cannot say which night it
+ // is, so a night that does know its wake replaces it once that night settles.
final sameNight = current != null &&
current.day == today &&
- (current.wakeSec == null ||
- wakeSec == null ||
- (wakeSec - current.wakeSec!).abs() < _headlineFreezeMarginSec);
+ (wakeSec == null ||
+ (current.wakeSec != null &&
+ (wakeSec - current.wakeSec!).abs() < kPinWakeToleranceSec));
if (sameNight) return current; // pinned; hold
- if (overnightComplete && liveReadiness != null) {
</code_context>
<issue_to_address>
**Ineligible nights keep old pins**
When a later derive makes a night ineligible while its wake remains within the pin tolerance, `nextFrozenHeadline` returns a same-wake pin before checking `pinnableNight`, and `headlinePinFor` validates only the wake match. The old pin remains the headline for an ineligible night, so Home and other pin readers keep showing its score.
Check the newly derived night’s eligibility before retaining a pin, and apply the eligibility check when reading a pin.
Also at `lib/compute/derivation_engine.dart:2181`, `lib/data/db.dart:3801-3804`.
</issue_to_address>
### Comment 4
<location path="lib/compute/derivation_engine.dart" line_range="2123-2131" />
<code_context>
+ required int dataEdgeSec,
+ int? nowSec,
+}) {
+ if (overnightSettled(
+ sleepOffsetSec: wakeSec, dataEdgeSec: dataEdgeSec, nowSec: nowSec)) {
+ return RecoveryState.finalReady;
+ }
+ if (wakeSec != null && dataEdgeSec >= wakeSec + kWakeConfirmMarginSec) {
+ return RecoveryState.provisional;
</code_context>
<issue_to_address>
**Awake mornings say sleeping**
When a today row exists without a sleep window and its data edge has not yet met the no-window settle condition, `recoveryStateOf` treats a missing `wakeSec` as `night_in_progress` whenever `overnightSettled` is false, and `refreshComputeFreshness` stores that state for any today row. `todayHeadlineOf` then tells Home and Coach the user is sleeping even when no sleep window has been detected.
Represent an absent sleep window separately from an in-progress night, and only report `night_in_progress` when there is evidence that sleep is underway.
Also at `lib/data/db.dart:10847-10853`.
</issue_to_address>
### Comment 5
<location path="lib/data/db.dart" line_range="3795-3800" />
<code_context>
+ final pin = await frozenHeadline();
+ final pinWake = pin?.wakeSec;
+ if (pin == null || pin.day != day || pinWake == null) return null;
+ int? wake;
+ try {
+ final w = jsonDecode(await sleepWindowJsonFor(day) ?? '{}');
+ final ms = w is Map ? w['offset_ms'] : null;
+ wake = ms is num ? ms ~/ 1000 : null;
+ } catch (_) {/* malformed window → no wake → no pin */}
+ if (wake == null || (wake - pinWake).abs() >= kPinWakeToleranceSec) {
+ return null;
</code_context>
<issue_to_address>
**Valid pins are rejected**
When the stored `window_json` has an outer `value` map containing `offset_ms`, `headlinePinFor` reads `offset_ms` only at the top level of `window_json`. When the stored window uses the `{value: {offset_ms: ...}}` envelope, it treats the wake as missing and rejects a valid pin, so Home falls back to a live readiness value that can drift after later derives.
Unwrap the `value` map when reading the wake, as `storedWindowSpan` does, before checking pin tolerance.
</issue_to_address>
### Comment 6
<location path="lib/state/app_state.dart" line_range="2268-2271" />
<code_context>
reanalyzeProgress = 'Analyzing…';
notifyListeners();
try {
+ // The user asked for a fresh answer: today's morning pin would otherwise
+ // hold the old number in the ring while the re-analysis summary reports
+ // the new one. The re-derive below re-pins once the night is final.
+ await LocalDb.releaseFrozenHeadline(LocalDb.localDayLabelNow());
final n = await _derive.run(
_profile,
</code_context>
<issue_to_address>
**Re-analysis skips its fresh pass**
When a sync or background derivation already holds the derivation latch when the user starts re-analysis, `reanalyzeAll` releases today’s pin before `_derive.run`, which returns `0` when another derive holds the latch. The requested pass is skipped after the pin is removed, so the headline can remain unpinned or an active pass can restore its older value while the UI reports completion.
Ensure re-analysis waits for or queues a derive that will run before releasing today’s pin.
</issue_to_address>
### Comment 7
<location path="lib/data/db.dart" line_range="3817-3841" />
<code_context>
+ /// has not changed since it was first shown.
+ static Future<Map<String, int>?> noteHeadlineShown(
+ String day, int value) async {
+ Map? cur;
+ try {
+ final d = jsonDecode(await getCursor(kHeadlineShownCursor) ?? 'null');
+ if (d is Map && d['day'] == day && d['value'] is num) cur = d;
+ } catch (_) {/* malformed → start over */}
+ if (cur != null && (cur['value'] as num).round() == value) {
+ final prev = cur['prev'], at = cur['at'];
+ return prev is num && at is num
+ ? {'from': prev.round(), 'to': value, 'at': at.toInt()}
+ : null;
+ }
+ final now = DateTime.now().millisecondsSinceEpoch;
+ await setCursor(
</code_context>
<issue_to_address>
**Recovery update cursor goes backward**
When overlapping `getToday` calls observe different final values during a re-analysis or late sync, `noteHeadlineShown` reads and later writes the shared cursor without a transaction or lock. An older snapshot can overwrite a newer cursor, so the next recovery update note reports an incorrect `from` value.
Make the cursor read-and-write atomic so stale snapshots cannot overwrite newer values.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 7 findings to address first, and if the new state and pin guards are wrong, the app can persist or serve an incorrect recovery headline and may trigger a notification based on that value; reverting will not undo already-written pins, cursors, or notifications. The stored headline data is bounded and can be recomputed, but externally shown or sent notifications cannot be fully retracted.
Blocking findings: lib/coach/coach_engine.dart:805, lib/coach/coach_db.dart:505, lib/compute/derivation_engine.dart:2180, lib/compute/derivation_engine.dart:2131, lib/data/db.dart:3800, and 2 more
Contributor
PR Code Suggestions ✨Latest suggestions up to ae9df7d Explore these optional code suggestions:
Previous suggestionsSuggestions up to commit f939cd3
|
Collaborator
Author
|
@coderabbitai review |
Contributor
|
- no sleep window yet claims no state instead of "Sleeping..." - headlinePinFor unwraps a value-enveloped window_json - Coach SQL masks today's readiness when getToday fails, and shows a pinned headline even when metric_series has no row for the day - re-analysis waits out a running derive before releasing the pin - the shown-headline cursor is read and written in one transaction - freeze test uses local wall-clock wakes (the 03:00 pin gate is local)
Contributor
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
# Conflicts: # lib/l10n/app_ru.arb
A re-derive that shrinks the same night below a main night (same wake, later onset) no longer keeps its old pin, and headlinePinFor checks the stored window's eligibility before returning one.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
Refs #543 and the "Home shows 2 while Coach says 27.6" report.
Root cause: Home showed a frozen headline pinned from a night still in progress (04:04 local, night 01:05-07:56), and the first pin of the day stuck; Coach read the stored day result.
todayHeadlineOf) for Home, Readiness detail, Health caption, briefing, notifications, widgets (provisional drawn muted) and Coach (get_today;v_daily/v_metricserve today's readiness through the same rule).Tests: recovery_states, recovery_headline_ui, overnight_settled, widget_service_sentinels, coach_views, low_readiness tests.
🤖 Generated with Claude Code
Summary by Sourcery
Unify today’s recovery headline around explicit settlement states while preventing incomplete nights and stale pins from producing incorrect scores.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests:
Chores:
PR Type
Bug fix, Enhancement
Description
Unify today's recovery headline handling across Home, Coach, Widgets, and Briefing
Introduce explicit recovery states:
night_in_progress,provisional, andfinalPrevent early pins from freezing the headline and trust pins only for matching wake times
Show an update note when a final recovery value changes after being displayed
Add
get_todaytool to Coach and shadowv_daily/v_metricto align with the headlineDiagram Walkthrough
File Walkthrough
12 files
Update briefing to use todayHeadlineOf for readinessShadow v_daily and v_metric with temp views for today's readinessAdd get_today tool and pass today's readiness to runCoachSqlUpdate getToday to include provisional readiness and update notesAdd todayHeadlineOf to centralize recovery headline logicShow changed recovery days after manual re-analysisAdd recoveryStateLine to show recovery statusHandle provisional state and show recovery update notesUpdate to use todayHeadlineOf and handle provisional stateHandle provisional readiness state for widgetsAdd English strings for recovery states and updatesAdd Russian strings for recovery states and updates1 files
Update prompt to instruct using get_today() for today's recovery3 files
Add RecoveryState enum, recoveryStateOf, and pinnableNight logicAdd headlinePinFor and noteHeadlineShown to manage recovery pinsRelease frozen headline during manual re-analysis4 files
Add tests for v_daily and v_metric shadowingAdd UI tests for recovery states and update notesAdd tests for recoveryStateOf and pin guardsAdd test for provisional night in widgets3 files