fix: stop the app freezing during backfills, add real crash/jank visibility#85
Merged
Conversation
…bility Two separate problems, found by actually wiring the app up to Firebase. The freeze/hang: derivation_engine.dart's cross-day rollup reads up to 90 days of stored results, decodes each one, rebuilds a cross-day record for it, and re-encodes the result — none of that ran off the main thread. On an older phone this blocked the UI for several seconds during a heavy derive pass. Moved the decode/transform/encode work into Isolate.run (the two helper functions it uses were already static, so this was a clean move) and did the same for the crossday bundle's own jsonEncode, which was happening back on the main isolate after the isolate call returned. The rebuild storm: several screens (Today, Steps, Spot Check, Profile's device sheet, breathing, the status banner, coach cards) used a blanket context.watch<AppState>() and rebuilt on all 67 of AppState's notifyListeners() calls, including three separate per-second timers. Switched these to context.select() scoped to the fields each screen actually reads. Reverted the same change on the main Profile screen after it broke the telemetry toggle — that screen touches too many fields across too many helper methods to track by hand safely, and it isn't one of the screens background timers actually hit, so watch() stays there. Also fixed a live-session map crash: fitCamera() could get called before the map had a real rendered size, and flutter_map's zoom math divides by that size — a zero-size viewport can produce a NaN/Infinite zoom that doesn't throw until the tile layer tries to use it a frame later. Added a guard on the viewport size before calling fitCamera. Added actual observability: Crashlytics breadcrumbs on navigation and derive passes, custom keys for screen/derive state, a frame-timing watchdog that reports slow frames as non-fatal issues (this is what caught the hang above), and a readiness diagnostic that records which of HRV/RHR/resp/temp were missing when the readiness score comes back empty. flutter analyze: clean. flutter test: 457/457.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (16)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe PR adds consent-gated telemetry for navigation, jank, derivation timing, and failures; surfaces readiness-absence diagnostics; moves cross-day JSON processing into isolates; names routes; and narrows selected Flutter ChangesObservability and performance updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AppShell
participant MaterialApp
participant Navigator
participant TelemetryNavigatorObserver
participant TelemetryService
participant Crashlytics
AppShell->>TelemetryService: install jank watchdog
AppShell->>MaterialApp: register navigator observer
Navigator->>TelemetryNavigatorObserver: navigation lifecycle event
TelemetryNavigatorObserver->>TelemetryService: set screen context and breadcrumb
TelemetryService->>Crashlytics: record consent-gated telemetry
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
lib/compute/derivation_engine.dart (1)
1368-1386: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNested
contextmap will render as an unqueryable string in Firebase Analytics.
record(kind: 'event', ..., context: (absentDiag as Map).cast<String, dynamic>())passes a map whose values (hrv,rhr,resp,temp) are themselves maps.TelemetryService.record()'s Analytics branch only special-casesnum/Stringand otherwise falls back tov.toString(), so each of those keys will show up in Firebase Analytics as a raw{value: true, baseline_n: 5}-style string instead of filterable fields. Breadcrumb/Crashlytics logging is unaffected (plain string), but the Analytics event loses most of its diagnostic value.Consider flattening before calling
record(), e.g.'hrv_value': ..., 'hrv_baseline_n': ...per input, so each field is independently filterable.🤖 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 1368 - 1386, Flatten absentDiag’s nested per-input diagnostic maps before passing context to TelemetryService.record in the readiness-absent block, using independently named fields such as each input’s value and baseline count. Preserve the existing breadcrumb message, event metadata, and once-per-day throttling while ensuring context contains only Analytics-supported scalar values.lib/telemetry/telemetry_service.dart (1)
364-394: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
current_screenwill often just report the route type, not the screen.
_nameOffalls back toroute.runtimeType.toString()whensettings.nameis unset. Several screens in this codebase push with plainMaterialPageRoute(builder: ...)/themedRoute(...)(noRouteSettings.name), socurrent_screenon a crash/ANR report will read something generic likeMaterialPageRoute<dynamic>for most navigations instead of the actual destination — undercutting the diagnostic value this observer is meant to add.Consider passing
RouteSettings(name: ...)at the more common push sites (or wrappingthemedRoute/similar helpers to auto-derive a name) socurrent_screenis actually screen-specific.🤖 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/telemetry/telemetry_service.dart` around lines 364 - 394, Ensure routes passed through TelemetryNavigatorObserver carry screen-specific RouteSettings.name values instead of relying on the generic runtime-type fallback. Update the common MaterialPageRoute/themedRoute navigation helpers or their call sites to provide meaningful destination names, while preserving existing named-route behavior in _nameOf and the observer callbacks.
🤖 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.
Nitpick comments:
In `@lib/compute/derivation_engine.dart`:
- Around line 1368-1386: Flatten absentDiag’s nested per-input diagnostic maps
before passing context to TelemetryService.record in the readiness-absent block,
using independently named fields such as each input’s value and baseline count.
Preserve the existing breadcrumb message, event metadata, and once-per-day
throttling while ensuring context contains only Analytics-supported scalar
values.
In `@lib/telemetry/telemetry_service.dart`:
- Around line 364-394: Ensure routes passed through TelemetryNavigatorObserver
carry screen-specific RouteSettings.name values instead of relying on the
generic runtime-type fallback. Update the common MaterialPageRoute/themedRoute
navigation helpers or their call sites to provide meaningful destination names,
while preserving existing named-route behavior in _nameOf and the observer
callbacks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d094b783-42c3-4b45-83f2-8339a565940d
📒 Files selected for processing (15)
lib/app.dartlib/compute/derivation_engine.dartlib/compute/onehz_pipeline.dartlib/main.dartlib/state/app_state.dartlib/telemetry/telemetry_service.dartlib/ui/activity/live_session_screen.dartlib/ui/insights/coach_cards.dartlib/ui/profile/advanced_data_screen.dartlib/ui/profile/profile_screen.dartlib/ui/screens/screens.dartlib/ui/spotcheck/spot_check_screen.dartlib/ui/stress/calm_breathing_screen.dartlib/ui/today/today_screen.dartlib/ui/widgets/status_banner.dart
Two things it flagged, both real:
The readiness-absent diagnostic was passing a nested map straight into
TelemetryService.record(), which only keeps num/String context values
as-is and stringifies everything else. So each of HRV/RHR/resp/temp
would've shown up in Firebase Analytics as one unqueryable
"{value: true, baseline_n: 5}"-style string instead of separate
filterable fields. Flattened it into hrv_value/hrv_baseline_n/etc.
current_screen (the custom key TelemetryNavigatorObserver sets on push)
falls back to the route's runtime type when there's no RouteSettings
name, and nothing in this codebase was naming its routes — so in
practice every crash/ANR report would just say something like
"MaterialPageRoute<void>" instead of which screen it actually happened
on. Added an optional name parameter to themedRoute() and passed a real
name at all 24 push sites across the app. Today screen's shared _push()
helper (used by 15 of those) now reads the destination widget's runtime
type once before pushing, so its many callers didn't need touching
individually.
flutter analyze: clean. flutter test: 457/457.
This was referenced Jul 18, 2026
Merged
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.
This pull request introduces a comprehensive telemetry and observability upgrade throughout the app, with a focus on improved error reporting, app state context, and performance tracing. It adds a new
TelemetryServiceAPI with breadcrumbs, context, non-fatal error reporting, and custom trace support, and wires it into key flows such as navigation, derivation, and UI jank detection. Several main-isolate performance bottlenecks are also addressed by offloading heavy JSON encoding/decoding to isolates. Additionally, minor state management improvements and bug fixes are included.Telemetry and Observability Improvements:
TelemetryServiceAPI providing methods for logging breadcrumbs, setting persistent context, recording non-fatal errors, and wrapping code in Firebase Performance traces. Also adds a frame jank watchdog to report UI freezes as non-fatal Crashlytics events. (lib/telemetry/telemetry_service.dart) [1] [2]TelemetryServiceinto the app's navigation via a newTelemetryNavigatorObserver(wired intoMaterialApp.navigatorObservers), and into top-level route changes in_Gate, so every screen transition is logged and visible in Crashlytics context. (lib/app.dart,lib/telemetry/telemetry_service.dart) [1] [2] [3] [4]lib/state/app_state.dart) [1] [2] [3]lib/compute/derivation_engine.dart,lib/compute/onehz_pipeline.dart) [1] [2] [3]Performance and Main-Isolate Responsiveness:
lib/compute/derivation_engine.dart) [1] [2] [3]Minor Improvements and Bug Fixes:
context.watchtocontext.selectin the live session screen to avoid unnecessary widget rebuilds. (lib/ui/activity/live_session_screen.dart)These changes significantly improve the app's ability to surface, diagnose, and resolve issues in production by providing much richer telemetry and by reducing the risk of main-isolate freezes during heavy computation.
Summary by CodeRabbit
New Features
Bug Fixes