fix the crashes coming in from crashlytics - #212
Conversation
opening Journey could throw and leave the timeline chart blank. the activity bands clamped their right edge using the left edge as the lower bound, and the time projection saturates at the plot width, so a workout that started in the last pixel of the day inverted the clamp. that fails for the whole chart, not just the one band, and it repeats on every repaint. the geometry is its own function now so the edge cases are pinned. sharing a data export from the history screen died on iOS 26 with "sharePositionOrigin: argument must be set". the share sheet is a popover there now, not just on iPad. the other share buttons already passed an anchor but fell back to null when the box was missing, and null is the same crash — they all go through one helper that returns a real rect. respiratory rate on the day curve re-ran its estimate once per heartbeat instead of once every five minutes. the cadence cursor only advanced when the estimate came back present, and daytime readings are movement-confounded so absent is the normal case. on a noisy day that burned minutes of cpu and blew the ninety second budget for the second half of a day's derive, which is why days were landing with a readiness but no naps, workouts, recovery or curves. the all-day hrv curve had the same shape plus a window sum that ran before its own gate. a day that lost its second half then overwrote the complete version of itself. the write replaces the whole row, and re-deriving deliberately revisits days that were already finished, so a timeout on a re-run destroyed detail that was computed correctly days earlier. it carries the previous detail forward now. values measured as absent stay absent. a headless start paced its first derive as if the app were on screen. the background flag defaults to foreground and otherwise only moves on a transition, so nothing ever set it for a wake that begins in the background. map tiles that fail with no network are no longer reported as app errors, and uncaught network failures generally are no longer filed as crashes. a dropped tile fetch was counting against crash-free users and burying real crashes.
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR updates derivation recovery and cadence logic, classifies transient platform errors, centralizes share-sheet positioning, and adds guards for invalid UI geometry, disposed widgets, and map tile-provider lifecycle. ChangesDerivation and UI robustness
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DerivationEngine
participant HRVCurve
participant RespiratoryCurve
participant Persistence
DerivationEngine->>HRVCurve: apply cadence gate and compute HRV points
DerivationEngine->>RespiratoryCurve: attempt respiratory estimation
HRVCurve-->>DerivationEngine: return accepted points
RespiratoryCurve-->>DerivationEngine: return available points
DerivationEngine->>Persistence: persist recovered detail and effective states
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
lib/compute/derivation_engine.dart (1)
3980-4005: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd cadence regression tests.
Test that HRV advances its cursor when RMSSD fails the plausibility limit. Test that respiratory estimation advances its cursor when
rsaRespRateis absent. Verify that each path runs once per cadence interval, not once per beat.As per coding guidelines: “Behavior changes, especially regressions involving readiness, abstention, idempotence, synchronization, migrations, and lifecycle safety, must include regression tests.”
Also applies to: 4042-4052
🤖 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 3980 - 4005, Add regression tests for the cadence behavior in the HRV derivation flow around the RMSSD plausibility check and the respiratory estimation flow around missing rsaRespRate. Verify both paths advance their cursor even when no output is emitted, including when RMSSD exceeds the limit or rsaRespRate is absent, and assert each path executes once per cadence interval rather than once per beat.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 930-933: Initialize the production DerivationEngine instance with
the constructor’s initial _background value so its final background-dependent
pacing fields are correct from the first sweep. Update the testing constructor
to explicitly initialize DerivationEngine with background false, and avoid
relying on the later _deriveScheduler.setBackground call for initial engine
state.
In `@lib/telemetry/error_classification.dart`:
- Around line 19-28: Update the transient exception classification around
transientTypes so TimeoutException is not treated as transient solely by type.
Remove it from the unconditional set and classify timeout errors as transient
only when their cause or available context identifies a network operation, while
preserving the existing handling for the other network exception types.
In `@lib/ui/profile/profile_screen.dart`:
- Around line 226-232: Guard both share handlers after their asynchronous work:
in lib/ui/profile/profile_screen.dart lines 226-232, check rowCtx.mounted after
LocalDb.exportCopy() before Share.shareXFiles and before showing the error
snackbar; in lib/ui/recap/recap_screen.dart lines 215-217, check mounted after
file.writeAsBytes() before Share.shareXFiles. Return immediately when the
relevant widget is unmounted.
---
Outside diff comments:
In `@lib/compute/derivation_engine.dart`:
- Around line 3980-4005: Add regression tests for the cadence behavior in the
HRV derivation flow around the RMSSD plausibility check and the respiratory
estimation flow around missing rsaRespRate. Verify both paths advance their
cursor even when no output is emitted, including when RMSSD exceeds the limit or
rsaRespRate is absent, and assert each path executes once per cadence interval
rather than once per beat.
🪄 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: d5efbec3-36cf-4f73-bc78-41d410548c87
📒 Files selected for processing (17)
lib/compute/derivation_engine.dartlib/state/app_state.dartlib/telemetry/error_classification.dartlib/telemetry/telemetry_service.dartlib/ui/activity/workout_share_card.dartlib/ui/coach/coach_render.dartlib/ui/coach/coach_settings_screen.dartlib/ui/kit/charts.dartlib/ui/kit/kit.dartlib/ui/kit/route_map.dartlib/ui/kit/share_origin.dartlib/ui/pairing_screen.dartlib/ui/profile/data_history_screen.dartlib/ui/profile/profile_screen.dartlib/ui/recap/recap_screen.dartlib/ui/timeline/timeline_screen.darttest/crash_regressions_test.dart
PR Reviewer Guide 🔍(Review updated until commit a9511fe)Here are some key observations to aid the review process:
|
PR Code Suggestions ✨ |
the derive engine's background flag is final and picks the concurrency and the per-day timeout, so seeding only the scheduler left a headless first sweep on the foreground budget. it is constructed with the initial value now, which was the actual gap — the scheduler seed alone did not fix what i said it fixed. the hrv curve advances its cursor before the usable-pairs check, not after. the window holds several hundred beats, so a stretch too noisy to yield eight clean pairs redid that whole sum on every following beat. timeouts are no longer transient by type. a derivation or database timeout raises the same class as a network one and should stay visible as a crash. both remaining share sheets check mounted after their export or capture, so a route that closed during the work does not present one.
|
all four were real, thanks. fixed in 6a1acb8. the engine one is the important one and it means my original fix did not do what the hrv cursor is out of the timeouts are out of the type set. a derivation, database or lifecycle timeout both share sheets check mounted after their export or capture. on the cadence tests — i added them but they are weaker than they look and i what they do not cover is the per-beat path itself. i tried to build a failing |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/crash_regressions_test.dart`:
- Around line 217-237: Add a regression test covering the
absent-respiratory-estimate path in DerivationEngine.dayRespCurve by introducing
a deterministic seam for ana.rsaRespRate or exposing an attempt counter. Assert
that an est.present == false attempt advances the five-minute cursor and that a
subsequent usable window emits a respiration point, while preserving the
existing clean-data spacing assertion.
🪄 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: 889d2076-b8ea-4c83-bf27-4f1284f75c17
📒 Files selected for processing (6)
lib/compute/derivation_engine.dartlib/state/app_state.dartlib/telemetry/error_classification.dartlib/ui/profile/profile_screen.dartlib/ui/recap/recap_screen.darttest/crash_regressions_test.dart
|
Persistent review updated to latest commit 6a1acb8 |
PR Code Suggestions ✨ |
the estimator abstains on real movement-confounded rr, and no synthetic rr i could build reproduced that, so the absent branch went uncovered. both curves now carry a test seam: an override for the respiratory estimator and an attempt counter on each curve. the fix is about how often the work runs rather than what it returns, so counting attempts is the only thing that separates the two versions. with the estimator forced to abstain across a two hour stretch, respiration attempts 7970 times before the fix and 26 after. the hrv window sum runs 5990 times before and 102 after.
|
good call, that was the right answer to the gap i described. done in 8a815c9. both curves now carry a seam: an override for the respiratory estimator and an forcing the estimator to abstain across a two hour stretch:
i checked both by reverting each cursor move in turn and confirming the test that also puts a number on the original claim, which i had only argued from the |
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 (1)
lib/compute/derivation_engine.dart (1)
2532-2562: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not promote an old version after a second-half failure.
LocalDb.dayResult(day.date)returns the latestalgo_version, so on a first v60 derive with a v59 row, recovery can carry v59 detail into the v60bundle, persist it asalgoVersion: kAlgoVersion, and restore the v59 finalized state. If the v60 second half never completes again, the v60 row is final but still contains v59 curves/detail. Only restorepartial = falseandfinalized = truewhenexisting['algo_version'] == kAlgoVersion. Keep cross-version recovery partial and unfinalized, and add a persisted-row regression test for this path.🤖 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 2532 - 2562, Restrict the recovery logic in the derivation flow around _isRealDayResult, carryForwardDetail, and effectiveFinalized so partial=false and finalized=true are restored only when existing['algo_version'] matches kAlgoVersion. Keep cross-version recovery detail carry-forward partial and unfinalized, and add a persisted-row regression test covering a v60 derive recovering from a v59 result after second-half failure.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 2532-2562: Restrict the recovery logic in the derivation flow
around _isRealDayResult, carryForwardDetail, and effectiveFinalized so
partial=false and finalized=true are restored only when existing['algo_version']
matches kAlgoVersion. Keep cross-version recovery detail carry-forward partial
and unfinalized, and add a persisted-row regression test covering a v60 derive
recovering from a v59 result after second-half failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 692870ed-8fb5-42d2-b6eb-5c14fb48b824
📒 Files selected for processing (2)
lib/compute/derivation_engine.darttest/crash_regressions_test.dart
|
Persistent review updated to latest commit 8a815c9 |
PR Code Suggestions ✨ |
workout_share_card read the anchor rect after the frame wait, the raster, the png encode and the file write. the helper's whole contract is that it runs while layout is still stable, and this was the one call site that did not. it does not crash — the fallback covers a detached box — but the rect can describe a box that has since moved.
|
right, fixed in 991a303. workout_share_card read the anchor after the frame |
|
Persistent review updated to latest commit 991a303 |
PR Code Suggestions ✨ |
the day lookup returns the highest stored algo_version, so on the first derive after a bump it hands back the row that is being replaced. if the second half then failed, that older detail was carried into the new bundle, written under the new version number and marked finished — locking in exactly the curves the bump exists to recompute, with nothing left to trigger another pass. a carry-forward from an older version now stays partial and unfinalized, so a later pass recomputes it properly. an import still force-finalizes, since there is no stored raw to recompute it from.
|
this one was the best catch of the review — fixed in a9511fe. it is specific to this pr bumping the algo version, and it would have hit every a cross-version carry now stays partial and unfinalized so a later pass redoes one thing worth flagging: writing this i nearly introduced a regression of my the decision is a pure function so the cases are pinned directly, and there is a |
|
Persistent review updated to latest commit a9511fe |
PR Code Suggestions ✨No code suggestions found for the PR. |
|
@CodeRabbit review |
✅ Action performedReview finished.
|
The branch forked at 5faa4b0 and was 96 commits behind. The last two experimental releases (0.9.22, 0.9.23) were both cut from that fork point, so WHOOP 5 testers have been running builds without fixes main has had for weeks — including the ones that matter most on iOS: strength workouts never reaching Apple Health (#184), strain scored 0.0 after a backgrounded workout (#206), reconnect dying for the process lifetime after one throw (#208), and the crash batch in #212, whose Journey-timeline clamp is the most-hit issue in Crashlytics. This merge ends that drift before the next experimental. Five conflicts, three of them real, all resolved as keep-both: getBattery/getHello — take main's throttled _pollBatteryIfDue (a raw send here was 2,880 radio round-trips a day) and keep the branch's gen5 HELLO, which is a different opcode on Maverick. enableHrOnlyLive — both sides wanted a line at the same place: the branch's isGen5 lookup and main's _applyLinkPriority() step-down. Both are kept. app_state imports — union of the two `show` lists. One conflict git resolved silently and wrongly, caught by the analyzer rather than by the merge: gen5 changed `setAlarm` to return the armed instant (null = the write never reached the band) where main returns a bool, and main's alarm grace-retry — code the branch has never seen — assigned it straight to `bool rearmed`. Same signal either way, so the call site becomes `!= null` and the retry bookkeeping is unchanged. Dependency pins, both moved forward rather than merged blind: protocol -> 367d22b, protocol main @ #16 merge. gen5 is ON MAIN now, so the side-branch pin is obsolete; this SHA is a strict superset of the 7edcb3e edge main carried (crc8 length check, realtimeRr RR-bound, odd-length hex rejection) plus every gen5 decoder the branch needs. analytics -> main's 1fb34dc. The branch's cbbe06a is an ancestor of it, 15 commits behind, so this is a straight fast-forward. flutter analyze lib test clean. Full suite +1535 ~2, 0 failures.
1 participant
User description
went through what crashlytics has been collecting since 0.9.14 and fixed what is
still live in 0.9.24.
opening Journey could throw and leave the timeline chart blank. the activity
bands clamped their right edge using the left edge as the lower bound, and the
time projection saturates at the plot width, so a workout that started in the
last pixel of the day inverted the clamp. that takes out the whole chart, not
just the one band, and it repeats on every repaint — which is why the event
count is so much higher than the number of people hitting it. the geometry is
its own function now so the edge cases are pinned. five other charts had the
same shape of clamp where the upper bound can fall below the lower one on a
narrow layout; those are fixed too, along with a coach hypnogram that could
build a negative-width rect from a model-authored spec.
sharing a data export from the history screen died on iOS 26 with
"sharePositionOrigin: argument must be set". the share sheet is a popover on
iPhone there now, not just on iPad. the other share buttons already passed an
anchor but fell back to null when the render box was missing — null is the same
crash, and so is a zero rect, so they all go through one helper that returns a
real one.
respiratory rate on the day curve re-ran its estimate once per heartbeat instead
of once every five minutes. the cadence cursor only advanced when the estimate
came back present, and daytime readings are movement-confounded so absent is the
normal case, not the exception. on a noisy day that burns minutes of cpu inside
the isolate that has ninety seconds to finish the second half of a day's derive.
that timeout is the most widely hit issue on the list and this is why. the
all-day hrv curve had the same shape, plus a window sum that ran before its own
cadence gate.
worse, a day that lost its second half then overwrote the complete version of
itself. the write replaces the whole row, and re-deriving deliberately revisits
days that were already finished, so a timeout on a re-run destroyed naps,
workouts, recovery and curves that had been computed correctly days earlier.
that one is silent — no crash, the day just gets thinner. it carries the previous
detail forward now, keyed on absence rather than null so a value that was
genuinely measured as absent stays absent.
a headless start paced its first derive as if the app were on screen. the
background flag defaults to foreground and otherwise only moves on a transition,
so nothing ever set it for a wake that begins in the background. the same gap
was fixed for the ble link a while back, one line above.
map tiles that fail with no network are no longer reported as app errors, and
uncaught network failures generally are no longer filed as crashes. a dropped
tile fetch was counting against crash-free users and burying the real ones. the
tile provider is also rebuilt when the map remounts, because the layer closes
its http client on dispose and an empty-then-refilled route came back with a
dead one.
algo version goes to 60: the two curve fixes change what lands in the stored
bundle.
a few things i looked at and left alone. the anr cluster is not the derive
pipeline starving the main thread — the substrate paging is split across page
reads that each yield, and the two hangs that really were main-isolate blocks
are already fixed. there is real waste there (one day's substrate is loaded
three times over overlapping windows) but it is a performance issue, not the
cause of the anrs, and i would rather not claim a fix i cannot demonstrate. the
readiness_absent non-fatals are all from builds before 0.9.19 and cannot be
emitted by current code. the platform-channel oom and the trig anrs are fixed in
what is already shipping.
still open: making the per-day deadlines survive ios suspending the app, and
giving the scheduler a way to re-pace work that is already in flight. both need
more thought than a patch.
PR Type
Bug fix, Enhancement
Description
Fixes derivation timeouts and data loss.
kAlgoVersionto 60.Fixes iOS 26 share sheet crashes.
Fixes UI rendering crashes.
clampbounds in charts.Improves crash reporting accuracy.
Diagram Walkthrough
File Walkthrough
12 files
Fix derivation timeouts and prevent data lossFix headless start pacingUse safe share origin for workoutsFix negative-width rects and inverted clampsAdd mounted guards after async gapsFix inverted clamp bounds in tooltipsFix map tile disposal and silence network errorsAdd mounted guard after bluetooth checkUse safe share origin for data exportsUse safe share origin for profile exportsUse safe share origin for recap sharingFix inverted clamp bounds in timeline charts2 files
Add transient network error classificationReport transient errors as non-fatal1 files
Export share_origin helper1 files
Add safe share origin helper for iOS 261 files
Add regression tests for fixed crashesSummary by CodeRabbit