diff --git a/lib/compute/derivation_engine.dart b/lib/compute/derivation_engine.dart index fce9c58..5e0fcc8 100644 --- a/lib/compute/derivation_engine.dart +++ b/lib/compute/derivation_engine.dart @@ -597,7 +597,18 @@ import 'substrate.dart'; // persisted bundle does, so days derived at v58 must be re-derived to pick it // up. `ABSENT` was deliberately NOT invented as a fifth tier — `Tier.all` in // analytics is a closed set of four published grades. -const int kAlgoVersion = 59; +// +// v60 - the all-day HRV and respiratory curves advance their cadence cursor on +// every ATTEMPT rather than only on a successful estimate. `_dayRespCurve` +// left `lastEmit` unset whenever rsaRespRate came back absent — and absent is +// the EXPECTED daytime case, because daytime RSA is movement-confounded — so a +// confounded stretch re-ran the triple Lomb-Scargle once per beat instead of +// once per five minutes. That is what exhausted the 90 s day-blocks budget and +// left days persisted headline-only. `_dayHrvCurve` had the same shape plus an +// O(window) sum that ran before its cadence gate was checked. Both curves keep +// their sampling intent; points that were previously emitted a beat or two +// after a failed attempt now land on the next cadence tick instead. +const int kAlgoVersion = 60; // Fold idempotency, the minimum-nights warm-up, and legacy-payload handling // all live in SleepProfilePolicy (pure, unit-tested) — see @@ -2508,6 +2519,38 @@ class DerivationEngine { final finalized = !producedNothing && (forceFinalize || (ageFinalized && secondHalfOk)); + // A failed/timed-out second half yields a headline-only bundle, and + // putDayResult replaces the row wholesale — so re-deriving an already + // complete day (rescanRecent deliberately revisits finalized days) destroyed + // its naps, sleep periods, workouts, HRR, wear and curves. Carry the + // previous result's detail forward instead of blanking it. Same principle as + // the producedNothing guard above and the skip-marker guard in + // _markDaySkipped: never let a thinner result overwrite a richer one. + var effectiveFinalized = finalized; + var effectivePartial = !secondHalfOk; + if (!secondHalfOk) { + final existing = await LocalDb.dayResult(day.date); + if (_isRealDayResult(existing)) { + final prev = _decodeBundle(existing!['payload_json']); + if (prev != null) { + final recovered = carryForwardDetail(prev, bundle); + final prevVersion = (existing['algo_version'] as num?)?.toInt(); + final outcome = recoveryOutcome( + recovered: recovered, + prevPartial: (existing['partial'] as num?)?.toInt() == 1, + prevVersion: prevVersion, + prevFinalized: (existing['finalized'] as num?)?.toInt() == 1, + finalizedByAge: finalized, + ); + effectivePartial = outcome.partial; + effectiveFinalized = outcome.finalized; + _log('derive ${day.date}: second half failed — carried the previous ' + "result's detail blocks forward (v$prevVersion -> v$kAlgoVersion, " + 'partial=$effectivePartial)'); + } + } + } + final scalars = (bundle['scalars'] as Map?)?.cast() ?? const {}; double? sc(String k) => (scalars[k] as num?)?.toDouble(); @@ -2518,8 +2561,8 @@ class DerivationEngine { windowJson: jsonEncode( ((day.sleepJson['window'] as Map?) ?? const {}).cast(), ), - finalized: finalized, - partial: !secondHalfOk, + finalized: effectiveFinalized, + partial: effectivePartial, rhr: sc('rhr'), rmssd: sc('rmssd'), readiness: sc('readiness'), @@ -2722,6 +2765,76 @@ class DerivationEngine { return row['rhr'] != null || row['rmssd'] != null || row['readiness'] != null; } + /// Fill [next]'s missing detail from [prev] when the second-half compute + /// failed, so a headline-only pass never blanks a day that already had naps, + /// workouts, HRR, wear and curves. Returns true if anything was carried over. + /// + /// Keyed on ABSENCE, not on null: isolate 1 writes its headline scalars + /// explicitly and a null there is a real "we could not measure this today" + /// that must survive. Only keys the failed second half never got to add are + /// restored — a freshly computed value always wins. + @visibleForTesting + static bool carryForwardDetail( + Map prev, + Map next, + ) { + var carried = false; + for (final e in prev.entries) { + if (e.key == 'scalars' || e.key == 'series') continue; + if (next.containsKey(e.key) || e.value == null) continue; + next[e.key] = e.value; + carried = true; + } + // `scalars` and `series` are flat maps the second half patches INTO rather + // than owning, so they merge per key instead of wholesale. + for (final sub in const ['scalars', 'series']) { + final p = prev[sub]; + if (p is! Map) continue; + final n = next[sub]; + if (n is! Map) { + next[sub] = Map.from(p.cast()); + carried = true; + continue; + } + for (final e in p.entries) { + if (n.containsKey(e.key) || e.value == null) continue; + n[e.key] = e.value; + carried = true; + } + } + return carried; + } + + /// How a day should be filed after its second half failed and the previous + /// result's detail was carried forward. + /// + /// The version check is the subtle part. `LocalDb.dayResult` returns the + /// HIGHEST algo_version stored for the day, so immediately after a bump the + /// row it hands back belongs to the previous version. Carrying that detail + /// forward still beats blanking the day — but it must not be filed as a + /// finished CURRENT-version result, because the reason a bump exists is that + /// those blocks are computed differently now. A cross-version carry therefore + /// stays partial and unfinalized, so a later pass recomputes it for real + /// instead of locking last version's curves in under this version's number. + @visibleForTesting + static ({bool partial, bool finalized}) recoveryOutcome({ + required bool recovered, + required bool prevPartial, + required int? prevVersion, + required bool prevFinalized, + required bool finalizedByAge, + }) { + final sameVersion = prevVersion == kAlgoVersion; + if (!recovered || prevPartial || !sameVersion) { + // Stays partial, but keep whatever the caller had already decided about + // finalizing: an IMPORT force-finalizes even a partial day, because there + // is no stored raw to ever recompute it from. + return (partial: true, finalized: finalizedByAge); + } + // As complete as it was before this pass, so it keeps what it had earned. + return (partial: false, finalized: finalizedByAge || prevFinalized); + } + /// Test seam for [_markDaySkipped] — the "a skip marker must never destroy a /// real result" guarantee is the whole point of the method, so it is pinned /// directly rather than through a full derive pass. @@ -3879,7 +3992,13 @@ class DerivationEngine { /// Timeline graph. 5-min sliding window, emitted ~each minute. Inline artifact /// gate (plausible RR 300–2000 ms) — daytime RR is noisier/motion-confounded, /// so this is a context line, not the nocturnal recovery RMSSD. - static List> _dayHrvCurve(Substrate s) { + /// Test seam: counts window evaluations, for the same reason as + /// [debugRespAttempts] — the fix is about how often the O(window) sum runs. + @visibleForTesting + static int debugHrvAttempts = 0; + + @visibleForTesting + static List> dayHrvCurve(Substrate s) { final ts = [], rr = []; for (var i = 0; i < s.rrMs.length; i++) { final v = s.rrMs[i]; @@ -3897,7 +4016,11 @@ class DerivationEngine { while (ts[i] - ts[lo] > winMs) { lo++; } - if (i - lo >= 10) { + // Cadence gate FIRST: the sum-of-squared-differences below is O(window), + // and running it for every beat only to discard the result on the 60 s + // check was the whole window's work wasted per sample. + if (i - lo >= 10 && ts[i] - lastEmit > 60000) { + debugHrvAttempts++; var ssd = 0.0; var nd = 0; for (var k = lo + 1; k <= i; k++) { @@ -3909,14 +4032,18 @@ class DerivationEngine { ssd += d * d; nd++; } - if (nd >= 8 && ts[i] - lastEmit > 60000) { + // Advance on the ATTEMPT, before either quality check. The window holds + // ~300-600 beats, and leaving the cursor behind when a stretch is too + // artifact-heavy to yield 8 usable pairs re-runs that whole sum on every + // subsequent beat until one finally does. + lastEmit = ts[i]; + if (nd >= 8) { final rmssd = math.sqrt(ssd / nd); if (rmssd <= 220) { out.add({ 't': (ts[i] / 1000).round(), 'v': double.parse(rmssd.toStringAsFixed(1)), }); - lastEmit = ts[i]; } } } @@ -3928,7 +4055,24 @@ class DerivationEngine { /// 24/7 RR. 3-min window emitted ~every 5 min; absent windows (too few/too /// noisy beats) are skipped — never fabricated. Daytime RSA is movement- /// confounded, so it's a context line. - static List> _dayRespCurve(Substrate s) { + /// + /// Test seam: replaces the RSA estimator, so the ABSENT branch — the one that + /// used to strand the cadence cursor and re-run a triple Lomb-Scargle per beat + /// — can be exercised deterministically. It needs a seam because no synthetic + /// RR reliably makes the real estimator abstain: the behaviour comes from real + /// movement-confounded daytime data, which is exactly what is hard to fake. + @visibleForTesting + static double? Function(List nn, List nnt)? + debugRespEstimator; + + /// Test seam: counts estimator ATTEMPTS. The cost fix is about how often the + /// estimator runs, not about what it returns, so the attempt count is the only + /// thing that actually distinguishes the fixed code from the broken code. + @visibleForTesting + static int debugRespAttempts = 0; + + @visibleForTesting + static List> dayRespCurve(Substrate s) { final ts = [], rr = []; for (var i = 0; i < s.rrMs.length; i++) { final v = s.rrMs[i]; @@ -3951,14 +4095,26 @@ class DerivationEngine { final nn = rr.sublist(lo, i + 1); final t0 = ts[lo]; final nnt = [for (var k = lo; k <= i; k++) ts[k] - t0]; - final est = ana.rsaRespRate(nn, nnt, artifactFraction: 0.15); - final brpm = est.present ? est.value!.brpm : null; + debugRespAttempts++; + final seam = debugRespEstimator; + final double? brpm; + if (seam != null) { + brpm = seam(nn, nnt); + } else { + final est = ana.rsaRespRate(nn, nnt, artifactFraction: 0.15); + brpm = est.present ? est.value!.brpm : null; + } + // Advance the cadence cursor on every ATTEMPT, not just on a successful + // estimate. Daytime RSA is movement-confounded (see above), so absent is + // the common case — and while lastEmit sat inside the success branch a + // confounded stretch re-ran the triple Lomb-Scargle once per BEAT + // instead of once per 5 min. That is what blew the day-blocks budget. + lastEmit = ts[i]; if (brpm != null) { out.add({ 't': (ts[i] / 1000).round(), 'v': double.parse(brpm.toStringAsFixed(1)), }); - lastEmit = ts[i]; } } } @@ -4534,8 +4690,8 @@ class DerivationEngine { } bundlePatch['daytime_hrv'] = _daytimeHrv(daySub, onset, offset); - seriesPatch['hrv_day'] = _dayHrvCurve(daySub); - seriesPatch['resp_day'] = _dayRespCurve(daySub); + seriesPatch['hrv_day'] = dayHrvCurve(daySub); + seriesPatch['resp_day'] = dayRespCurve(daySub); seriesPatch['skin_temp_day'] = _daySkinTempCurve(daySub); bundlePatch['restlessness'] = _restlessness(sleepSub); // napSub extends a few hours past this day's calendar end so a nap/ diff --git a/lib/state/app_state.dart b/lib/state/app_state.dart index d40609d..0fc1914 100644 --- a/lib/state/app_state.dart +++ b/lib/state/app_state.dart @@ -127,7 +127,12 @@ class AppState extends ChangeNotifier { /// The on-device compute orchestrator. Kicked (light) after every drain/flush /// completion, and (heavy) on foreground finalize. Background heavy passes run /// via WorkManager (Android) — see lib/compute/background_derivation.dart. - late final DerivationEngine _derive = DerivationEngine(log: _log); + // `background` is final on the engine and picks the concurrency + per-day + // timeout, so seeding the scheduler alone left a headless first sweep running + // the foreground budget. Late-initialized, so this reads the value both + // constructors have already set by the time anything touches `_derive`. + late final DerivationEngine _derive = + DerivationEngine(log: _log, background: _background); late final DeriveScheduler _deriveScheduler = DeriveScheduler( run: ({required DeriveJobKind kind}) => _afterDrain(heavy: kind == DeriveJobKind.heavy), @@ -927,6 +932,10 @@ class AppState extends ChangeNotifier { // connection interval would run at the fast one until the user next // foregrounded the app. engine.setBackground(_background); + // Same reasoning for the derive pacing budget: it defaults to foreground and + // otherwise only flips on a transition, so a headless start paced its very + // first sweep as if the app were on screen. + _deriveScheduler.setBackground(_background); repo = LocalRepositoryImpl(getProfileMap: () => user); // iOS BGProcessing/BGAppRefresh wakes while the FOREGROUND app owns the band // skip the headless BLE path (it would fight FBP for the peripheral) — route diff --git a/lib/telemetry/error_classification.dart b/lib/telemetry/error_classification.dart new file mode 100644 index 0000000..fd51418 --- /dev/null +++ b/lib/telemetry/error_classification.dart @@ -0,0 +1,41 @@ +// Which uncaught errors actually represent a crash. +// +// `PlatformDispatcher.onError` was filing everything it saw as fatal. That is +// wrong for the transient network failures the app is expected to survive: a +// basemap tile fetched with no connectivity, a TLS handshake dropped mid-flight, +// a request abandoned when its widget went away. None of those stop the app, but +// each one counted against the crash-free-user rate and buried real crashes in +// the issue list. +// +// Pure and dependency-free so it can be unit tested without a Flutter binding. + +/// True when [error] is a transient I/O failure rather than a defect. +/// +/// Deliberately matched on type NAME rather than on an imported type: this file +/// stays dependency-free, and the relevant types come from `dart:io` and +/// `package:http`, which classify identically here. +bool isTransientError(Object error) { + final type = error.runtimeType.toString(); + const transientTypes = { + 'SocketException', + 'HttpException', + 'HandshakeException', + 'TlsException', + 'ClientException', + 'ConnectionClosedException', + }; + // TimeoutException is deliberately NOT here. It is not network-specific — a + // derivation, database or lifecycle timeout raises the same type, and those + // are exactly the failures worth keeping visible as crashes. + if (transientTypes.contains(type)) return true; + + // http's ClientException wraps the underlying cause in its message rather + // than nesting the exception, so the wrapped form has to be matched on text. + final text = error.toString(); + return text.contains('SocketException') || + text.contains('HandshakeException') || + text.contains('Connection closed') || + text.contains('Connection reset') || + text.contains('Failed host lookup') || + text.contains('Network is unreachable'); +} diff --git a/lib/telemetry/telemetry_service.dart b/lib/telemetry/telemetry_service.dart index 160cc3c..691b2c2 100644 --- a/lib/telemetry/telemetry_service.dart +++ b/lib/telemetry/telemetry_service.dart @@ -27,6 +27,7 @@ import 'package:firebase_performance/firebase_performance.dart'; import 'package:firebase_analytics/firebase_analytics.dart'; import '../cloud/companion_client.dart'; +import 'error_classification.dart'; import 'jank_policy.dart'; /// A band-side snapshot AppState supplies (it owns the live DeviceState). @@ -167,7 +168,15 @@ class TelemetryService { PlatformDispatcher.instance.onError = (Object error, StackTrace stack) { try { if (Firebase.apps.isNotEmpty && _enabled) { - FirebaseCrashlytics.instance.recordError(error, stack, fatal: true); + // Same reasoning as the `silent` check above, for the errors the + // framework never sees: a dropped tile fetch or a TLS handshake that + // died with no network is not a crash, and filing it as one both + // understates crash-free users and buries the real crashes. + FirebaseCrashlytics.instance.recordError( + error, + stack, + fatal: !isTransientError(error), + ); } } catch (_) {} record(kind: 'crash', level: 'error', message: '$error', stack: '$stack'); diff --git a/lib/ui/activity/workout_share_card.dart b/lib/ui/activity/workout_share_card.dart index 7bcfbe0..0bf37bc 100644 --- a/lib/ui/activity/workout_share_card.dart +++ b/lib/ui/activity/workout_share_card.dart @@ -40,6 +40,7 @@ import '../../state/units_controller.dart'; import '../../theme/theme.dart'; import '../../theme/tokens.dart'; import '../kit/kit.dart' show AppIcon, OsIcon; +import '../kit/share_origin.dart'; import '../kit/route_map.dart'; /// Where the image is going. Strava-style: a feed post and a story, because @@ -337,6 +338,10 @@ class _WorkoutSharePreviewScreenState extends State { Future _share() async { if (_sharing) return; + // Measured before the capture/encode/write awaits below, per shareOriginFor: + // after an async gap this widget may have been relaid out and the rect would + // describe a box that has moved. + final origin = shareOriginFor(context); setState(() => _sharing = true); try { final boundary = _captureKey.currentContext?.findRenderObject() @@ -382,10 +387,6 @@ class _WorkoutSharePreviewScreenState extends State { await file.writeAsBytes(bytes.buffer.asUint8List()); if (!mounted) return; - final box = context.findRenderObject() as RenderBox?; - final origin = (box != null && box.hasSize) - ? (box.localToGlobal(Offset.zero) & box.size) - : null; // No caption text: the image carries everything, and a canned // "My OpenStrap workout" string is exactly the kind of filler that makes // a share feel automated. diff --git a/lib/ui/coach/coach_render.dart b/lib/ui/coach/coach_render.dart index 39ca7a7..1906a5d 100644 --- a/lib/ui/coach/coach_render.dart +++ b/lib/ui/coach/coach_render.dart @@ -354,6 +354,10 @@ class _HypnogramPainter extends CustomPainter { final x0 = size.width * ((_num(s['start']) ?? 0) - lo) / (hi - lo); final x1 = size.width * ((_num(s['end']) ?? 0) - lo) / (hi - lo); if (x0 > clipW) continue; + // The segment list comes from a model-authored spec, so end < start is + // reachable and would build a negative-width rect (same guard the + // hand-built hypnogram already carries). + if (x1 <= x0) continue; final rect = Rect.fromLTWH(x0, lane * laneH + 2, math.min(x1, clipW) - x0, laneH - 4); canvas.drawRRect( @@ -424,7 +428,7 @@ class _RangeBand extends StatelessWidget { child: Container(height: 8, decoration: BoxDecoration(color: AppColors.good.withValues(alpha: 0.4), borderRadius: BorderRadius.circular(4)))), - Positioned(left: (at(value) - 6).clamp(0, w - 12), top: 4, + Positioned(left: (at(value) - 6).clamp(0, math.max(0, w - 12)), top: 4, child: Container(width: 12, height: 12, decoration: BoxDecoration(color: AppColors.coral, shape: BoxShape.circle))), ])); diff --git a/lib/ui/coach/coach_settings_screen.dart b/lib/ui/coach/coach_settings_screen.dart index 881706a..2b5dba0 100644 --- a/lib/ui/coach/coach_settings_screen.dart +++ b/lib/ui/coach/coach_settings_screen.dart @@ -53,11 +53,13 @@ class _CoachSettingsScreenState extends State { setState(() { _loadingModels = true; _msg = null; }); try { final ids = await CoachEngine.fetchModels(_base.text, _key.text); + if (!mounted) return; setState(() { _models = ids; _msg = ids.isEmpty ? 'Provider returned no models — type one manually.' : '${ids.length} models found. Search and tap to pick.'; }); } catch (e) { + if (!mounted) return; setState(() => _msg = e is CoachException ? e.message : 'Could not list models: $e'); } finally { if (mounted) setState(() => _loadingModels = false); diff --git a/lib/ui/kit/charts.dart b/lib/ui/kit/charts.dart index 859fac2..9452772 100644 --- a/lib/ui/kit/charts.dart +++ b/lib/ui/kit/charts.dart @@ -583,7 +583,10 @@ class _TimeSeriesChartState extends State { Positioned( left: (activeDx + 8).clamp( leftPad, - constraints.maxWidth - 120, + // A chart narrower than the tooltip leaves no room to + // slide it: the upper bound would fall below leftPad and + // clamp throws on inverted bounds. + math.max(leftPad, constraints.maxWidth - 120), ), top: 8, child: IgnorePointer( diff --git a/lib/ui/kit/kit.dart b/lib/ui/kit/kit.dart index aac8316..8c0700c 100644 --- a/lib/ui/kit/kit.dart +++ b/lib/ui/kit/kit.dart @@ -5,6 +5,7 @@ // The illustrated icon set rides along with the kit so every screen that // imports kit/design gets OsAppIcon + OsIcon without touching the package. export 'os_icons.dart'; +export 'share_origin.dart'; import 'package:flutter/material.dart'; import 'package:flutter/services.dart'; diff --git a/lib/ui/kit/route_map.dart b/lib/ui/kit/route_map.dart index ce8d083..767e38b 100644 --- a/lib/ui/kit/route_map.dart +++ b/lib/ui/kit/route_map.dart @@ -172,6 +172,20 @@ class _RouteMapViewState extends State { List _glow = const []; List _crisp = const []; + // TileLayer DISPOSES whatever provider it is handed when it leaves the tree, + // and NetworkTileProvider closes its internal client on dispose — so the + // instance cannot simply be held for the life of the State. The `pts.isEmpty` + // early return in build() tears the layer down, so a route that empties and + // refills (a live workout before its first accepted fix) would come back with + // a closed client and fail every tile from then on. Build a fresh one on each + // remount instead. + // + // `silenceExceptions` is the actual fix for the reported issue: a tile fetch + // that fails with no network resolves to a transparent tile instead of + // throwing into the zone error handler, which was reporting every failed + // basemap request as an app error. + NetworkTileProvider? _tiles; + @override void initState() { super.initState(); @@ -279,7 +293,13 @@ class _RouteMapViewState extends State { for (final p in _points) if (p.latitude.isFinite && p.longitude.isFinite) p, ]; - if (pts.isEmpty) return const SizedBox.shrink(); + if (pts.isEmpty) { + // The TileLayer is about to be disposed (and with it our provider); drop + // the reference so the next mount builds a live one. + _tiles = null; + return const SizedBox.shrink(); + } + final tileProvider = _tiles ??= NetworkTileProvider(silenceExceptions: true); // Detect gesture-driven camera moves so a live map can stop auto-following. void onPositionChanged(MapCamera camera, bool hasGesture) { @@ -341,6 +361,7 @@ class _RouteMapViewState extends State { urlTemplate: _kOsmTileUrl, subdomains: _kTileSubdomains, userAgentPackageName: _kUserAgent, + tileProvider: tileProvider, // NOT `maxZoom` — see kRouteMapMaxTileZoom. Leaving the display // ceiling at its default keeps tiles on screen at every zoom. maxNativeZoom: kRouteMapMaxTileZoom.round(), diff --git a/lib/ui/kit/share_origin.dart b/lib/ui/kit/share_origin.dart new file mode 100644 index 0000000..e413092 --- /dev/null +++ b/lib/ui/kit/share_origin.dart @@ -0,0 +1,22 @@ +import 'package:flutter/widgets.dart'; + +/// Anchor rect for the iOS share sheet. +/// +/// On iPad — and since iOS 26 on iPhone too — `UIActivityViewController` is +/// presented as a popover, and it rejects a missing origin AND a degenerate one +/// ("{{0, 0}, {0, 0}} must be non-zero and within coordinate space of source +/// view"). Passing `null` is therefore not a safe fallback: it is the crashing +/// input. When the render box is gone, unsized, or scrolled off screen, anchor +/// to the middle of the screen instead so the sheet still opens. +/// +/// Call this BEFORE any await. After one the widget may have been unmounted or +/// relaid out, and the rect would describe a box that no longer exists. +Rect shareOriginFor(BuildContext context) { + final screen = Offset.zero & MediaQuery.of(context).size; + final box = context.findRenderObject() as RenderBox?; + if (box != null && box.hasSize) { + final visible = (box.localToGlobal(Offset.zero) & box.size).intersect(screen); + if (visible.width > 0 && visible.height > 0) return visible; + } + return Rect.fromCenter(center: screen.center, width: 1, height: 1); +} diff --git a/lib/ui/pairing_screen.dart b/lib/ui/pairing_screen.dart index 00c423f..a83ac90 100644 --- a/lib/ui/pairing_screen.dart +++ b/lib/ui/pairing_screen.dart @@ -257,8 +257,9 @@ class _ScanStepState extends State<_ScanStep> { // Bluetooth being off is the #1 reason a scan silently finds nothing — // check it explicitly instead of letting a swallowed platform error // through as a misleading "no strap found." - if (!await app.bluetoothReady()) { - if (!mounted) return; + final ready = await app.bluetoothReady(); + if (!mounted) return; + if (!ready) { setState(() { _phase = PairPhase.bluetoothOff; _error = null; diff --git a/lib/ui/profile/data_history_screen.dart b/lib/ui/profile/data_history_screen.dart index 206d563..d0c0b54 100644 --- a/lib/ui/profile/data_history_screen.dart +++ b/lib/ui/profile/data_history_screen.dart @@ -66,11 +66,23 @@ class _DataHistoryScreenState extends State { } Future _shareWholeDb() async { + // Captured before the export await: the share sheet needs an anchor rect + // measured while this screen's layout is still stable. + final origin = shareOriginFor(context); setState(() => _busy = true); try { final path = await LocalDb.exportCopy(); if (!mounted) return; - await Share.shareXFiles([XFile(path)], text: 'OpenStrap data export'); + await Share.shareXFiles( + [XFile(path)], + text: 'OpenStrap data export', + sharePositionOrigin: origin, + ); + } catch (e) { + if (!mounted) return; + ScaffoldMessenger.of( + context, + ).showSnackBar(SnackBar(content: Text('Export failed: $e'))); } finally { if (mounted) setState(() => _busy = false); } @@ -79,6 +91,7 @@ class _DataHistoryScreenState extends State { Future _shareSelected() async { if (_selected.isEmpty) return; final app = context.read(); + final origin = shareOriginFor(context); setState(() => _busy = true); try { final path = await app.exportDaysDb(_selected); @@ -87,6 +100,7 @@ class _DataHistoryScreenState extends State { [XFile(path)], text: 'OpenStrap selected day export (${_selected.length} day${_selected.length == 1 ? '' : 's'})', + sharePositionOrigin: origin, ); } catch (e) { if (!mounted) return; diff --git a/lib/ui/profile/profile_screen.dart b/lib/ui/profile/profile_screen.dart index ad9a06c..c1eac89 100644 --- a/lib/ui/profile/profile_screen.dart +++ b/lib/ui/profile/profile_screen.dart @@ -223,15 +223,16 @@ class ProfileScreen extends StatelessWidget { value: 'Share', onTap: () async { final messenger = ScaffoldMessenger.of(rowCtx); - final box = rowCtx.findRenderObject() as RenderBox?; + final origin = shareOriginFor(rowCtx); try { final path = await LocalDb.exportCopy(); + // The export can outlive the route; presenting a share sheet + // for a screen the user already left is not wanted. + if (!rowCtx.mounted) return; await Share.shareXFiles( [XFile(path)], text: 'OpenStrap data export', - sharePositionOrigin: box != null - ? box.localToGlobal(Offset.zero) & box.size - : null, + sharePositionOrigin: origin, ); } catch (e) { messenger.showSnackBar( diff --git a/lib/ui/recap/recap_screen.dart b/lib/ui/recap/recap_screen.dart index 41c7acf..27c2003 100644 --- a/lib/ui/recap/recap_screen.dart +++ b/lib/ui/recap/recap_screen.dart @@ -212,11 +212,9 @@ class _RecapScreenState extends State { try { // iOS/iPad: the share sheet is a popover and REQUIRES an anchor rect, or // it throws PlatformException(sharePositionOrigin: argument must be set). - // Capture it now, before any async gap, while layout is stable. - final box = context.findRenderObject() as RenderBox?; - final origin = (box != null && box.hasSize) - ? (box.localToGlobal(Offset.zero) & box.size) - : null; + // Capture it now, before any async gap, while layout is stable. A null + // fallback would just be the crashing input again — see shareOriginFor. + final origin = shareOriginFor(context); final boundary = _cardKey.currentContext?.findRenderObject() @@ -234,6 +232,9 @@ class _RecapScreenState extends State { '${dir.path}/openstrap_recap_${DateTime.now().millisecondsSinceEpoch}.png', ); await file.writeAsBytes(bytes.buffer.asUint8List()); + // Capture and encode can outlive the route; don't present a share sheet + // for a screen the user already left. + if (!mounted) return; await Share.shareXFiles( [XFile(file.path)], diff --git a/lib/ui/timeline/timeline_screen.dart b/lib/ui/timeline/timeline_screen.dart index ce0503b..143fc08 100644 --- a/lib/ui/timeline/timeline_screen.dart +++ b/lib/ui/timeline/timeline_screen.dart @@ -23,6 +23,8 @@ // bundle itself, but the timeline is only ever reached embedded in the Journey // screen (which does its own loading), so that wrapper was removed. +import 'dart:math' as math; + import 'package:flutter/material.dart'; import '../design/design.dart'; @@ -82,6 +84,27 @@ double? plottedLineValueAt( return avg.last.v; } +/// Horizontal extent of one activity band, given the band's projected start/end +/// and the plot width. +/// +/// The painter's `x(t)` saturates at [width], so a band whose START falls in the +/// final pixel used to make the old `x1.clamp(x0 + 1, width)` inverted — +/// `double.clamp` throws `ArgumentError` when lowerLimit exceeds upperLimit, and +/// the whole chart then failed to paint ("Invalid argument(s): 330.77…" from +/// JourneyScreen). Pinning the left edge a pixel off the right and widening +/// forward keeps every band at least 1 px wide without ever inverting. +/// +/// Pure so the geometry is unit-testable without mounting the chart. +({double left, double right}) activityBandExtent( + double startX, + double endX, + double width, +) { + final left = math.min(startX, width - 1); + final right = math.min(math.max(endX, left + 1), width); + return (left: left, right: right); +} + class _Vital { final String label; final String unit; @@ -612,15 +635,19 @@ class _ChartPainter extends CustomPainter { // ── activity bands across the block + glyph on top ── for (final b in bands) { - final x0 = x(b.start), x1 = x(b.end); + // See [activityBandExtent] for why this is not a plain clamp. + final (left: x0, right: x1) = + activityBandExtent(x(b.start), x(b.end), size.width); canvas.drawRect( - Rect.fromLTRB(x0, plotTop, x1.clamp(x0 + 1, size.width), plotBot), + Rect.fromLTRB(x0, plotTop, x1, plotBot), Paint()..color = b.color.withValues(alpha: 0.10 * progress), ); canvas.drawLine(Offset(x0, plotTop), Offset(x1, plotTop), Paint()..color = b.color.withValues(alpha: 0.7)..strokeWidth = 2); if (progress > 0.6) { - final cx = ((x0 + x1) / 2).clamp(leftPad + 8, size.width - 8); + final cx = math.min( + math.max((x0 + x1) / 2, leftPad + 8), + math.max(leftPad + 8, size.width - 8)); canvas.drawCircle(Offset(cx, plotTop - 8), 4, Paint()..color = b.color); } } @@ -650,7 +677,8 @@ class _ChartPainter extends CustomPainter { final t = t0 + (t1 - t0) * f; final tx = leftPad + chartW * f; _text(canvas, _hhmm(t), - Offset((tx - 14).clamp(leftPad, size.width - 28), plotBot + 5), + Offset((tx - 14).clamp(leftPad, math.max(leftPad, size.width - 28)), + plotBot + 5), AppColors.inkMuted, 9); } @@ -771,7 +799,8 @@ class _ChartPainter extends CustomPainter { final label = '${up ? '↑' : '↓'} ${p.v.toStringAsFixed(v.decimals)} @${_hhmm(p.t)}'; _text(canvas, label, - Offset((px + 4).clamp(leftPad, maxW - 78), py + (up ? -14 : 5)), + Offset((px + 4).clamp(leftPad, math.max(leftPad, maxW - 78)), + py + (up ? -14 : 5)), v.color, 9); } diff --git a/test/crash_regressions_test.dart b/test/crash_regressions_test.dart new file mode 100644 index 0000000..3165189 --- /dev/null +++ b/test/crash_regressions_test.dart @@ -0,0 +1,367 @@ +// Regressions for the crashes reported through Crashlytics against 0.9.21. +// +// Each group pins the exact mechanism that reached production, so a future +// refactor that reintroduces it fails here rather than in the field. + +import 'package:flutter/material.dart'; +import 'package:flutter_test/flutter_test.dart'; + +import 'package:openstrap_edge/compute/derivation_engine.dart'; +import 'package:openstrap_edge/compute/substrate.dart'; +import 'package:openstrap_edge/telemetry/error_classification.dart'; +import 'package:openstrap_edge/ui/kit/share_origin.dart'; +import 'package:openstrap_edge/ui/timeline/timeline_screen.dart'; + +void main() { + group('activityBandExtent (the inverted clamp)', () { + // The painter used `x1.clamp(x0 + 1, size.width)`. x(t) saturates at + // size.width, so a band starting in the final pixel made lowerLimit exceed + // upperLimit and double.clamp threw — reported from JourneyScreen as + // "Invalid argument(s): 330.7788280945566" (the value IS x0 + 1). + const w = 330.7788280945566; + + void expectSane(({double left, double right}) e) { + expect(e.left, lessThanOrEqualTo(e.right), + reason: 'an inverted rect is the crash'); + expect(e.right, lessThanOrEqualTo(w)); + expect(e.right - e.left, greaterThanOrEqualTo(1.0), + reason: 'a band must stay visible'); + } + + test('a band starting exactly at the right edge', () { + expectSane(activityBandExtent(w, w, w)); + }); + + test('a band starting within the last pixel', () { + // The exact production case: x0 = 329.7788, so x0 + 1 > width. + expectSane(activityBandExtent(w - 0.2, w, w)); + }); + + test('a zero-length band mid-plot still gets a visible width', () { + final e = activityBandExtent(100, 100, w); + expect(e.right - e.left, 1.0); + }); + + test('an end before its start does not invert', () { + expectSane(activityBandExtent(200, 100, w)); + }); + + test('an ordinary band is left untouched', () { + final e = activityBandExtent(40, 200, w); + expect(e.left, 40); + expect(e.right, 200); + }); + + test('the old expression really did throw on these inputs', () { + // Pins WHY the function exists: without it, this is the production crash. + expect(() => w.clamp(w + 1, w), throwsArgumentError); + }); + }); + + group('shareOriginFor', () { + // iOS 26 rejects a missing origin AND a zero one, so `null` was never a + // safe fallback — it is the crashing input. + testWidgets('returns the widget rect when laid out', (tester) async { + late Rect origin; + await tester.pumpWidget(MaterialApp( + home: Builder(builder: (context) { + origin = shareOriginFor(context); + return const SizedBox(width: 100, height: 40); + }), + )); + expect(origin.width, greaterThan(0)); + expect(origin.height, greaterThan(0)); + }); + + testWidgets('falls back to a non-zero rect with no render box', + (tester) async { + late Rect origin; + await tester.pumpWidget(MaterialApp( + home: Builder(builder: (context) { + // A LayoutBuilder's context has no RenderBox of its own at this point. + origin = shareOriginFor(context); + return const SizedBox.shrink(); + }), + )); + expect(origin.width, greaterThan(0)); + expect(origin.height, greaterThan(0)); + expect(origin, isNot(Rect.zero)); + }); + }); + + group('isTransientError', () { + test('network failures are not crashes', () { + for (final e in [ + "ClientException with SocketException: Failed host lookup: " + "'c.basemaps.cartocdn.com' (OS Error: No address associated with " + 'hostname, errno = 7)', + 'SocketException: Network is unreachable (OS Error: Network is ' + 'unreachable, errno = 51)', + 'HandshakeException: Connection terminated during handshake', + 'ClientException: Connection closed while receiving data', + ]) { + expect(isTransientError(_Err(e)), isTrue, reason: e); + } + }); + + test('real defects still count as crashes', () { + expect(isTransientError(ArgumentError('330.77')), isFalse); + expect(isTransientError(StateError('readiness_absent')), isFalse); + expect(isTransientError(_Err('Null check operator used on a null value')), + isFalse); + }); + }); + + group('carryForwardDetail', () { + // A timed-out second half produced a headline-only bundle, and putDayResult + // replaces the row wholesale — so re-deriving a complete day destroyed its + // naps, workouts, HRR, wear and curves. + test('restores detail blocks the failed pass never produced', () { + final prev = { + 'scalars': {'readiness': 71.0, 'nap_min': 24.0}, + 'series': {'rhr': 52.0}, + 'naps': [ + {'start': 1, 'end': 2} + ], + 'sessions': [ + {'sport': 'run'} + ], + }; + final next = { + 'scalars': {'readiness': 68.0}, + 'series': {'rhr': 53.0}, + }; + + expect(DerivationEngine.carryForwardDetail(prev, next), isTrue); + expect(next['naps'], prev['naps']); + expect(next['sessions'], prev['sessions']); + // Second-half scalar restored... + expect((next['scalars'] as Map)['nap_min'], 24.0); + // ...but the freshly computed headline always wins. + expect((next['scalars'] as Map)['readiness'], 68.0); + expect((next['series'] as Map)['rhr'], 53.0); + }); + + test('a deliberate null is absence, not a hole to backfill', () { + // Isolate 1 succeeded and measured no readiness today. Carrying yesterday's + // value forward would fabricate a metric — the honesty contract forbids it. + final prev = { + 'scalars': {'readiness': 71.0}, + }; + final next = { + 'scalars': {'readiness': null}, + }; + DerivationEngine.carryForwardDetail(prev, next); + expect((next['scalars'] as Map)['readiness'], isNull); + }); + + test('reports nothing carried when the previous result was no richer', () { + final prev = { + 'scalars': {'readiness': 71.0}, + }; + final next = { + 'scalars': {'readiness': 68.0}, + }; + expect(DerivationEngine.carryForwardDetail(prev, next), isFalse); + }); + }); + + group('recoveryOutcome (cross-version carry-forward)', () { + // dayResult returns the HIGHEST algo_version stored, so right after a bump + // the row handed back is the previous version's. Carrying its detail into a + // current-version row and marking that finished would lock last version's + // curves in under this version's number — and the bump exists precisely + // because those curves are computed differently now. + test('a same-version carry restores completeness', () { + final r = DerivationEngine.recoveryOutcome( + recovered: true, + prevPartial: false, + prevVersion: kAlgoVersion, + prevFinalized: true, + finalizedByAge: false, + ); + expect(r.partial, isFalse); + expect(r.finalized, isTrue, reason: 'keeps the flag it had earned'); + }); + + test('a cross-version carry stays partial and unfinalized', () { + final r = DerivationEngine.recoveryOutcome( + recovered: true, + prevPartial: false, + prevVersion: kAlgoVersion - 1, + prevFinalized: true, + finalizedByAge: false, + ); + expect(r.partial, isTrue, + reason: 'a later pass must recompute it for real'); + expect(r.finalized, isFalse, + reason: 'never lock the previous version detail in as current'); + }); + + test('carrying from an already-partial row stays partial', () { + final r = DerivationEngine.recoveryOutcome( + recovered: true, + prevPartial: true, + prevVersion: kAlgoVersion, + prevFinalized: false, + finalizedByAge: false, + ); + expect(r.partial, isTrue); + }); + + test('an import still force-finalizes a partial day', () { + // There is no stored raw to recompute an import from, so forceFinalize + // must survive the version gate. + final r = DerivationEngine.recoveryOutcome( + recovered: true, + prevPartial: false, + prevVersion: kAlgoVersion - 1, + prevFinalized: false, + finalizedByAge: true, + ); + expect(r.partial, isTrue); + expect(r.finalized, isTrue); + }); + + test('nothing carried leaves the day partial', () { + final r = DerivationEngine.recoveryOutcome( + recovered: false, + prevPartial: false, + prevVersion: kAlgoVersion, + prevFinalized: true, + finalizedByAge: false, + ); + expect(r.partial, isTrue); + expect(r.finalized, isFalse); + }); + }); + + group('day curve cadence (the per-beat rework)', () { + // Both curves left their cadence cursor behind whenever an estimate came + // back unusable, so the next beat re-entered and redid the whole window. + // For respiration that window is a triple Lomb-Scargle, which is what + // exhausted the 90 s day-blocks budget. + Substrate subFromRr(List rrMs, {int startSec = 1786100000}) { + final tsMs = []; + var t = startSec * 1000.0; + for (final r in rrMs) { + t += r; + tsMs.add(t); + } + final secs = [for (var i = 0; i < 10; i++) startSec + i]; + return Substrate( + tsSec: secs, + hr: List.filled(secs.length, 60), + rrTsMs: tsMs, + rrMs: rrMs, + ax: List.filled(secs.length, 0), + ay: List.filled(secs.length, 0), + az: List.filled(secs.length, 1), + spo2Red: List.filled(secs.length, 0), + spo2Ir: List.filled(secs.length, 0), + skinTemp: List.filled(secs.length, 0), + skinContact: List.filled(secs.length, 0), + ); + } + + // ~2 hours of clean beats, then the emission count is bounded by the + // cadence. Before the fix an attempt could recur every beat; the cursor now + // moves on every attempt, so the count can never exceed span/cadence. + List clean(int n) => + [for (var i = 0; i < n; i++) 850.0 + (i % 7) * 10]; + + // Alternating long/short pairs trip the Malik 20% ectopic reject, so a + // window yields too few usable pairs to estimate — the branch that used to + // leave the cursor behind. + List artifacty(int n) => + [for (var i = 0; i < n; i++) i.isEven ? 400.0 : 1600.0]; + + test('hrv points are never closer than the 60 s cadence', () { + final pts = DerivationEngine.dayHrvCurve(subFromRr(clean(8000))); + expect(pts, isNotEmpty); + for (var i = 1; i < pts.length; i++) { + expect(pts[i]['t']! - pts[i - 1]['t']!, greaterThanOrEqualTo(60)); + } + }); + + test('respiration points are never closer than the 5 min cadence', () { + final pts = DerivationEngine.dayRespCurve(subFromRr(clean(8000))); + for (var i = 1; i < pts.length; i++) { + expect(pts[i]['t']! - pts[i - 1]['t']!, greaterThanOrEqualTo(300)); + } + }); + + test('an absent estimate still advances the 5 min cursor', () { + // THE regression. With the estimator forced to abstain, the fixed code + // attempts once per cadence interval; the old code attempted once per + // BEAT, because lastEmit only moved inside the success branch. + addTearDown(() { + DerivationEngine.debugRespEstimator = null; + DerivationEngine.debugRespAttempts = 0; + }); + DerivationEngine.debugRespEstimator = (_, _) => null; + DerivationEngine.debugRespAttempts = 0; + + final rr = clean(8000); // ~7000 s of beats at ~880 ms + final sub = subFromRr(rr); + final spanSec = rr.reduce((a, b) => a + b) / 1000; + final pts = DerivationEngine.dayRespCurve(sub); + + expect(pts, isEmpty, reason: 'an absent estimate must emit nothing'); + // One attempt per 5 min of span, plus a little slack. The old code would + // land in the thousands here. + final cadenceCeiling = (spanSec / 300).ceil() + 2; + expect(DerivationEngine.debugRespAttempts, + lessThanOrEqualTo(cadenceCeiling), + reason: 'the estimator must not re-run per beat while abstaining'); + expect(DerivationEngine.debugRespAttempts, greaterThan(1), + reason: 'it must still keep trying across the day'); + }); + + test('a usable window after an absent stretch still emits', () { + // Advancing on failure must not silence the curve once quality returns. + addTearDown(() { + DerivationEngine.debugRespEstimator = null; + DerivationEngine.debugRespAttempts = 0; + }); + var calls = 0; + DerivationEngine.debugRespEstimator = (_, _) { + calls++; + return calls <= 3 ? null : 14.5; + }; + final pts = DerivationEngine.dayRespCurve(subFromRr(clean(8000))); + expect(pts, isNotEmpty); + expect(pts.first['v'], 14.5); + }); + + test('an unusable stretch emits nothing and does not re-run per beat', () { + // Alternating long/short pairs trip the Malik reject, so no window ever + // reaches 8 usable pairs — the branch that used to strand the cursor. + addTearDown(() => DerivationEngine.debugHrvAttempts = 0); + DerivationEngine.debugHrvAttempts = 0; + final rr = artifacty(6000); + final spanSec = rr.reduce((a, b) => a + b) / 1000; + expect(DerivationEngine.dayHrvCurve(subFromRr(rr)), isEmpty); + expect(DerivationEngine.debugHrvAttempts, + lessThanOrEqualTo((spanSec / 60).ceil() + 2), + reason: 'the window sum must not re-run per beat while unusable'); + }); + + test('a clean stretch after an unusable one still produces points', () { + // Advancing on failure must not silence the curve once quality returns. + final pts = DerivationEngine.dayHrvCurve( + subFromRr([...artifacty(2000), ...clean(6000)]), + ); + expect(pts, isNotEmpty); + }); + }); +} + +/// Stands in for the platform exception types, which carry their identity in +/// their message the same way `ClientException` does. +class _Err implements Exception { + final String message; + const _Err(this.message); + @override + String toString() => message; +} diff --git a/test/derive_result_protection_test.dart b/test/derive_result_protection_test.dart index fc64444..33fed1f 100644 --- a/test/derive_result_protection_test.dart +++ b/test/derive_result_protection_test.dart @@ -301,4 +301,49 @@ void main() { reason: 'a skip marker carries no user data, so replacing one with ' 'another is fine — only REAL results are protected'); }); + + // ── 3. a version bump must not launder old detail into a finished row ────── + + test('dayResult hands back the PREVIOUS version row after a bump', () async { + // The precondition the cross-version guard rests on: the query is + // `ORDER BY algo_version DESC LIMIT 1`, with no filter to kAlgoVersion. So + // on the first derive after a bump, the row offered for carry-forward + // belongs to the version that is being replaced. + const day = '2026-04-02'; + await LocalDb.putDayResult( + dayId: day, + algoVersion: kAlgoVersion - 1, + payloadJson: jsonEncode({ + 'scalars': {'readiness': 74.0}, + // Exactly the blocks a bump exists to recompute. + 'series': {'resp_day': [], 'hrv_day': []}, + 'naps': [ + {'start': 1, 'end': 2} + ], + }), + windowJson: '{}', + finalized: true, + rhr: 52, + rmssd: 61, + readiness: 74, + series: const {'readiness': 74.0}, + ); + + final row = await LocalDb.dayResult(day); + expect((row!['algo_version'] as num).toInt(), kAlgoVersion - 1, + reason: 'the carry-forward source is the OLD version row'); + + // Which is why recovery must refuse to file that as a finished current + // result — otherwise the previous version curves lock in under this + // version number and are never recomputed. + final outcome = DerivationEngine.recoveryOutcome( + recovered: true, + prevPartial: (row['partial'] as num?)?.toInt() == 1, + prevVersion: (row['algo_version'] as num?)?.toInt(), + prevFinalized: (row['finalized'] as num?)?.toInt() == 1, + finalizedByAge: false, + ); + expect(outcome.partial, isTrue); + expect(outcome.finalized, isFalse); + }); }