From 21cf35aba6ecf40381dad85070924779e2fe73a9 Mon Sep 17 00:00:00 2001 From: abdulsaheel Date: Sat, 8 Aug 2026 16:53:06 +0530 Subject: [PATCH 1/5] fix the crashes coming in from crashlytics MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- lib/compute/derivation_engine.dart | 105 +++++++++++++- lib/state/app_state.dart | 4 + lib/telemetry/error_classification.dart | 39 ++++++ lib/telemetry/telemetry_service.dart | 11 +- lib/ui/activity/workout_share_card.dart | 6 +- lib/ui/coach/coach_render.dart | 6 +- lib/ui/coach/coach_settings_screen.dart | 2 + lib/ui/kit/charts.dart | 5 +- lib/ui/kit/kit.dart | 1 + lib/ui/kit/route_map.dart | 23 +++- lib/ui/kit/share_origin.dart | 22 +++ lib/ui/pairing_screen.dart | 5 +- lib/ui/profile/data_history_screen.dart | 16 ++- lib/ui/profile/profile_screen.dart | 6 +- lib/ui/recap/recap_screen.dart | 8 +- lib/ui/timeline/timeline_screen.dart | 39 +++++- test/crash_regressions_test.dart | 176 ++++++++++++++++++++++++ 17 files changed, 442 insertions(+), 32 deletions(-) create mode 100644 lib/telemetry/error_classification.dart create mode 100644 lib/ui/kit/share_origin.dart create mode 100644 test/crash_regressions_test.dart diff --git a/lib/compute/derivation_engine.dart b/lib/compute/derivation_engine.dart index fce9c58b..6cb9a704 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,35 @@ 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 prevPartial = (existing['partial'] as num?)?.toInt() == 1; + if (recovered && !prevPartial) { + // The day is now as complete as it was before this pass, so it is no + // longer partial and may keep the finalized flag it had earned. + effectivePartial = false; + effectiveFinalized = finalized || + (existing['finalized'] as num?)?.toInt() == 1; + } + _log('derive ${day.date}: second half failed — carried the previous ' + 'result\'s detail blocks forward (partial=$effectivePartial)'); + } + } + } + final scalars = (bundle['scalars'] as Map?)?.cast() ?? const {}; double? sc(String k) => (scalars[k] as num?)?.toDouble(); @@ -2518,8 +2558,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 +2762,46 @@ 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; + } + /// 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. @@ -3897,7 +3977,10 @@ 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) { var ssd = 0.0; var nd = 0; for (var k = lo + 1; k <= i; k++) { @@ -3909,14 +3992,17 @@ class DerivationEngine { ssd += d * d; nd++; } - if (nd >= 8 && ts[i] - lastEmit > 60000) { + if (nd >= 8) { + // Advance on a computed estimate, not only on one that passed the + // plausibility filter — otherwise a stretch of artifact-heavy beats + // re-runs the window every beat. + lastEmit = ts[i]; 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]; } } } @@ -3953,12 +4039,17 @@ class DerivationEngine { 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; + // 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]; } } } diff --git a/lib/state/app_state.dart b/lib/state/app_state.dart index d40609d3..dec127ee 100644 --- a/lib/state/app_state.dart +++ b/lib/state/app_state.dart @@ -927,6 +927,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 00000000..608f0776 --- /dev/null +++ b/lib/telemetry/error_classification.dart @@ -0,0 +1,39 @@ +// 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', + 'TimeoutException', + 'ConnectionClosedException', + }; + 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 160cc3cb..691b2c27 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 7bcfbe0b..f5673d3f 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 @@ -382,10 +383,7 @@ 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; + final origin = shareOriginFor(context); // 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 39ca7a7d..1906a5da 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 881706ad..2b5dba09 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 859fac23..9452772b 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 aac83165..8c0700c4 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 ce8d0831..767e38b6 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 00000000..e4130925 --- /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 00c423f4..a83ac900 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 206d5632..d0c0b548 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 ad9a06ca..f361d052 100644 --- a/lib/ui/profile/profile_screen.dart +++ b/lib/ui/profile/profile_screen.dart @@ -223,15 +223,13 @@ 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(); 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 41c7acf3..2cbc8d8e 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() diff --git a/lib/ui/timeline/timeline_screen.dart b/lib/ui/timeline/timeline_screen.dart index ce0503be..143fc080 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 00000000..70e861bb --- /dev/null +++ b/test/crash_regressions_test.dart @@ -0,0 +1,176 @@ +// 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/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); + }); + }); +} + +/// 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; +} From 6a1acb8f896d7b5b2710207282e7b212e3a80782 Mon Sep 17 00:00:00 2001 From: abdulsaheel Date: Sat, 8 Aug 2026 17:08:53 +0530 Subject: [PATCH 2/5] address review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- lib/compute/derivation_engine.dart | 19 ++++--- lib/state/app_state.dart | 7 ++- lib/telemetry/error_classification.dart | 4 +- lib/ui/profile/profile_screen.dart | 3 ++ lib/ui/recap/recap_screen.dart | 3 ++ test/crash_regressions_test.dart | 71 +++++++++++++++++++++++++ 6 files changed, 97 insertions(+), 10 deletions(-) diff --git a/lib/compute/derivation_engine.dart b/lib/compute/derivation_engine.dart index 6cb9a704..2dcd3d2e 100644 --- a/lib/compute/derivation_engine.dart +++ b/lib/compute/derivation_engine.dart @@ -3959,7 +3959,8 @@ 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) { + @visibleForTesting + static List> dayHrvCurve(Substrate s) { final ts = [], rr = []; for (var i = 0; i < s.rrMs.length; i++) { final v = s.rrMs[i]; @@ -3992,11 +3993,12 @@ class DerivationEngine { ssd += d * d; nd++; } + // 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) { - // Advance on a computed estimate, not only on one that passed the - // plausibility filter — otherwise a stretch of artifact-heavy beats - // re-runs the window every beat. - lastEmit = ts[i]; final rmssd = math.sqrt(ssd / nd); if (rmssd <= 220) { out.add({ @@ -4014,7 +4016,8 @@ 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) { + @visibleForTesting + static List> dayRespCurve(Substrate s) { final ts = [], rr = []; for (var i = 0; i < s.rrMs.length; i++) { final v = s.rrMs[i]; @@ -4625,8 +4628,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 dec127ee..0fc19149 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), diff --git a/lib/telemetry/error_classification.dart b/lib/telemetry/error_classification.dart index 608f0776..fd514183 100644 --- a/lib/telemetry/error_classification.dart +++ b/lib/telemetry/error_classification.dart @@ -22,9 +22,11 @@ bool isTransientError(Object error) { 'HandshakeException', 'TlsException', 'ClientException', - 'TimeoutException', '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 diff --git a/lib/ui/profile/profile_screen.dart b/lib/ui/profile/profile_screen.dart index f361d052..c1eac89e 100644 --- a/lib/ui/profile/profile_screen.dart +++ b/lib/ui/profile/profile_screen.dart @@ -226,6 +226,9 @@ class ProfileScreen extends StatelessWidget { 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', diff --git a/lib/ui/recap/recap_screen.dart b/lib/ui/recap/recap_screen.dart index 2cbc8d8e..27c2003f 100644 --- a/lib/ui/recap/recap_screen.dart +++ b/lib/ui/recap/recap_screen.dart @@ -232,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/test/crash_regressions_test.dart b/test/crash_regressions_test.dart index 70e861bb..f15244b5 100644 --- a/test/crash_regressions_test.dart +++ b/test/crash_regressions_test.dart @@ -7,6 +7,7 @@ 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'; @@ -164,6 +165,76 @@ void main() { expect(DerivationEngine.carryForwardDetail(prev, next), 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 unusable stretch emits nothing and still terminates', () { + // The cursor now advances on the ATTEMPT, so this cannot degrade into a + // per-beat re-run of the window. + expect(DerivationEngine.dayHrvCurve(subFromRr(artifacty(6000))), isEmpty); + }); + + 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 From 8a815c910a627709aefc0c99eab417a027082bcf Mon Sep 17 00:00:00 2001 From: abdulsaheel Date: Sat, 8 Aug 2026 18:17:10 +0530 Subject: [PATCH 3/5] pin the cadence fixes with tests that fail without them 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. --- lib/compute/derivation_engine.dart | 33 +++++++++++++++-- test/crash_regressions_test.dart | 58 +++++++++++++++++++++++++++--- 2 files changed, 85 insertions(+), 6 deletions(-) diff --git a/lib/compute/derivation_engine.dart b/lib/compute/derivation_engine.dart index 2dcd3d2e..23855175 100644 --- a/lib/compute/derivation_engine.dart +++ b/lib/compute/derivation_engine.dart @@ -3959,6 +3959,11 @@ 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. + /// 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 = []; @@ -3982,6 +3987,7 @@ class DerivationEngine { // 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++) { @@ -4016,6 +4022,22 @@ 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. + /// + /// 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 = []; @@ -4040,8 +4062,15 @@ 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 diff --git a/test/crash_regressions_test.dart b/test/crash_regressions_test.dart index f15244b5..1998a207 100644 --- a/test/crash_regressions_test.dart +++ b/test/crash_regressions_test.dart @@ -221,10 +221,60 @@ void main() { } }); - test('an unusable stretch emits nothing and still terminates', () { - // The cursor now advances on the ATTEMPT, so this cannot degrade into a - // per-beat re-run of the window. - expect(DerivationEngine.dayHrvCurve(subFromRr(artifacty(6000))), isEmpty); + 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', () { From 991a303b27bdacc23b0420479297bb63e41b025a Mon Sep 17 00:00:00 2001 From: abdulsaheel Date: Sat, 8 Aug 2026 18:31:24 +0530 Subject: [PATCH 4/5] measure the share anchor before the capture, not after MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- lib/ui/activity/workout_share_card.dart | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/ui/activity/workout_share_card.dart b/lib/ui/activity/workout_share_card.dart index f5673d3f..0bf37bc9 100644 --- a/lib/ui/activity/workout_share_card.dart +++ b/lib/ui/activity/workout_share_card.dart @@ -338,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() @@ -383,7 +387,6 @@ class _WorkoutSharePreviewScreenState extends State { await file.writeAsBytes(bytes.buffer.asUint8List()); if (!mounted) return; - final origin = shareOriginFor(context); // 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. From a9511feb8bc622f2b2bae3bce19ea889790b58fd Mon Sep 17 00:00:00 2001 From: abdulsaheel Date: Sat, 8 Aug 2026 19:03:01 +0530 Subject: [PATCH 5/5] do not file previous-version detail as a finished current-version day MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- lib/compute/derivation_engine.dart | 51 ++++++++++++++---- test/crash_regressions_test.dart | 70 +++++++++++++++++++++++++ test/derive_result_protection_test.dart | 45 ++++++++++++++++ 3 files changed, 157 insertions(+), 9 deletions(-) diff --git a/lib/compute/derivation_engine.dart b/lib/compute/derivation_engine.dart index 23855175..5e0fcc8a 100644 --- a/lib/compute/derivation_engine.dart +++ b/lib/compute/derivation_engine.dart @@ -2534,16 +2534,19 @@ class DerivationEngine { final prev = _decodeBundle(existing!['payload_json']); if (prev != null) { final recovered = carryForwardDetail(prev, bundle); - final prevPartial = (existing['partial'] as num?)?.toInt() == 1; - if (recovered && !prevPartial) { - // The day is now as complete as it was before this pass, so it is no - // longer partial and may keep the finalized flag it had earned. - effectivePartial = false; - effectiveFinalized = finalized || - (existing['finalized'] as num?)?.toInt() == 1; - } + 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 (partial=$effectivePartial)'); + "result's detail blocks forward (v$prevVersion -> v$kAlgoVersion, " + 'partial=$effectivePartial)'); } } } @@ -2802,6 +2805,36 @@ class DerivationEngine { 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. diff --git a/test/crash_regressions_test.dart b/test/crash_regressions_test.dart index 1998a207..3165189a 100644 --- a/test/crash_regressions_test.dart +++ b/test/crash_regressions_test.dart @@ -166,6 +166,76 @@ void main() { }); }); + 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. diff --git a/test/derive_result_protection_test.dart b/test/derive_result_protection_test.dart index fc644444..33fed1ff 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); + }); }