Live activity screen rebuild, background route recording, and a real share card - #162
Conversation
The gallery had drifted into a catalogue of things nothing used. Scanned every public type in ui/design + ui/kit for call sites outside its own file and the gallery, then removed what had none: RadialHeatmap, RecapCard, AreaSpark, DotMatrix, CalendarHeatmap, StatTile, BaselineProgress, NightCard, NavPillAction, StateChips, OrbitScore's satellite layer, and the dead TimelineScreen wrapper (Journey embeds TimelineContent and loads its own bundle). Kept ArcGaugePainter/SkelBox/RouteMapScreen/RouteZoneLegend/MetricInfo — they scan as unused but are consumed inside their own files. Where a deleted component carried a real invariant, the test moved rather than went: RecapCard's "a missing day holds its slot instead of sliding the week left" now runs against MiniBars, whose invariant it actually was. Also: the Today readiness ring's status word is now a StateChipView pill (Push / Focus / Recover) instead of loose text. Still derived from the SAME readinessBand() cuts the AI briefing uses, so the ring and the briefing cannot disagree. Presentation only — no kAlgoVersion bump. ToggleChip had 5 live call sites and no gallery section at all; it has one now. The gallery covers what ships, in both directions. -1060 lines.
Five independent ways a workout could die, found while chasing "the app closes in the middle" reports. Each is separately sufficient: 1. Android FGS type silently stripped. EdgeTrackingService.start(Context) builds its Intent with no EXTRA_LOCATION, defaulting to false — and CompanionBridge.onDeviceAppeared calls it whenever the band re-enters BLE range, which happens routinely mid-run from arm-swing dropouts. That re-startForeground'd as connectedDevice only, dropping `location` from a live session. The extra is now tri-state: present => that's the mode and it latches, absent => inherit what the live session asked for. The Dart-side sticky flag could never protect this; these callers never go through Dart. 2. Heavy derivation fired mid-ride. The foreground/background gate is INVERTED for this case: phone on the bars with the screen awake means the app IS foreground, so an isolate spawn (roughly doubling peak heap) landed at the worst possible moment, competing with GPS, the live map and the BLE drain. New DeriveScheduler.setWorkoutActive gate; a workout is minutes long and derives at the end anyway. 3. The screen slept, with no wakelock anywhere. Held now via the existing method channels — FLAG_KEEP_SCREEN_ON / isIdleTimerDisabled, both window-scoped, no new dependency. Released on every teardown path. 4. After a process restart, _reconcileOrphanedLiveWorkout restored the timer, calories and strain but never restarted route tracking — the map was silently dead for the rest of the session and you only found out at the finish screen. 5. notifyListeners() after dispose. Re-arming route tracking exposed the hazard dispose()'s own comment already warns about for timers: an in-flight await cannot be cancelled. Added a _disposed guard rather than weakening the test that caught it. Also caps the decoded-image cache at 40 MiB. Flutter's 100 MiB default is sized for a photo feed; retina map tiles are ~1 MiB decoded each, so a long ride could sit on ~100 MiB of tile bitmaps on top of the persistent pre-warmed engine, which is LMK/jetsam territory on a 3-4 GB device. Worth knowing: telemetry is opt-in and defaults OFF, so none of this necessarily produced a crash report. Worth querying Crashlytics for jank_watchdog on LiveSessionScreen from consenting users.
REVIEW THIS ONE ON ITS OWN — it changes a user-facing privacy promise.
Without the `location` UIBackgroundMode, iOS suspends the process within
seconds of a screen lock or an app switch. The fix stream dies, the route
is lost, and a suspended app is first in line for jetsam. That was the
single largest cause of "the app closed mid-ride". The previous v1 stance
— while-in-use only plus a "Keep the screen on to map your route" hint in
the UI — was the architecture admitting the gap in UI copy, and it could
not survive a real 40-minute workout.
Scope is deliberately tight, and each of these was verified rather than
assumed:
- Authorization stays WHEN-IN-USE. geolocator_apple's PermissionHandler
is an if/else if: because we ship NSLocationWhenInUseUsageDescription
it calls requestWhenInUseAuthorization and the Always branch is
unreachable. We never ask for always-on location.
- Session-scoped. _settings() has exactly one caller (stream()), which
has exactly one caller (_maybeStartRouteTracking), torn down on every
workout-end path.
- showBackgroundLocationIndicator is ON, so the blue pill is visible the
entire time we read location in the background.
- Routes still go only to the on-device workout_route table. This
changes WHEN we can read GPS, not where any of it goes.
The old usage string said "OpenStrap does not track your location in the
background." That becomes false the moment this ships, so it could not
stay. Both strings rewritten to describe what actually happens.
Both privacy documents also gain a "Location and workout routes" section
— neither mentioned location AT ALL, which was already a gap since the
app has recorded routes for a while. The claim that the AI Coach cannot
read route data is literal: coach_db derives its allowed root-page set by
EXPLAINing the permitted views, so workout_route is unreachable at the
btree level on a read-only handle, not merely absent from a name list.
No App Store / Play privacy-label change is needed: Apple's definition of
"collect" is transmission off device, and data processed only on device
is explicitly excluded. Android uses FOREGROUND_SERVICE_LOCATION, not
ACCESS_BACKGROUND_LOCATION, so no Play background-location declaration is
triggered either. App Review (guideline 2.5.4) is the thing to satisfy,
via the purpose strings above.
… bugs
The overlapping elements were structural, not styling. The screen was one
flat Stack of absolutely-positioned layers with no layout relationship
between them, so collisions were guaranteed on some device: the map's
re-centre button was pinned `bottom: 96` while the control panel is far
taller than that, so it rendered UNDERNEATH it; the centred recording pill
ran under the 44 px map toggle; the fixed 270 px core had nothing stopping
it colliding with the clock above and the panel below on a short phone.
Now a bounded hero and a metric sheet as SIBLINGS in a Column — overlap is
impossible rather than merely unlikely. Things that genuinely float are
Positioned inside the hero's own Stack, anchored to the hero's edges. The
ring takes its size from a LayoutBuilder instead of a hard 270.
Sheet follows the hierarchy every production run/ride app converges on:
one primary figure readable from a bar mount, a zone-tinted HR pill, three
evenly-weighted secondary stats. The old panel gave six stats identical
weight and tagged every one with the SAME generic pulse icon, so nothing
read first and the icons carried no information.
Bugs fixed alongside:
- Pace read "40:32 /km" after barely moving: average pace divided
distance by ELAPSED time, so every second spent standing still made
it worse. Moving time only now; "—" when there is none. The test
reproduces the exact reported number.
- The 5-minute confetti re-fired on every return to the screen — the
fired-milestone set lived on the screen's State, which is disposed
and rebuilt on navigation. It belongs to the workout, so the workout
holds it now.
- The map greyed out past a zoom: TileLayer.maxZoom means "above this,
draw NOTHING" (its docs say leave it infinite); maxNativeZoom is the
one that describes a tile source and scales instead. Swapped. Also
enabled retinaMode — the URL carried the {r} placeholder but the flag
was never set, so we fetched standard-res tiles and upscaled them on
every high-DPI device.
- Zone colours were invisible. Measured, not eyeballed: the ramp
resolved the ACTIVE palette while this screen is always dark, and Z0
mapped to `cool` — a SURFACE token — measuring 1.03:1 against
nightAlt. Z0 is the RESTING zone, so that is what is on screen at the
start of every workout. New zoneOnDark ramp, Z0 at 9.76:1, guarded by
a test asserting all six clear 3:1.
- Label text used Colors.white30 (2.72:1) where small text needs 4.5:1.
onNight/onNightSoft already existed and already passed — the screen
just wasn't using them. Added the missing onNightMuted step so there
is a token for every role and no reason to reach for a raw whiteNN.
The layout test asserts against the BOTTOM-most hero element. An earlier
version asserted on the clock and passed with the bug deliberately
reintroduced; this one fails by 15.5 px, verified both ways.
Sharing rasterised the whole finish card — header, route thumbnail, strain
gauge, peak/avg/kcal/steps, time-in-zones, the HR-recovery curve and any
PR badges — into one tall PNG and handed it straight to the OS sheet. A
screenshot of a dashboard, and you never saw it before it hit the
composer.
The card is now composed for the job: map full-bleed owning the frame, one
headline figure, three supporting stats, nothing else. Post (4:5) and
Story (9:16). The scrim is bottom-weighted at 52% height on purpose — a
full-height gradient greys the whole map out, which is what makes these
look muddy. Indoor workouts get the same composition with a zone-washed
backdrop rather than a second design.
Preview before send: the card is on screen at real proportions with a
format switcher. Better UX, and it removes a real failure mode —
capturing a map that was never on screen races tile loading and yields
half-blank images.
Share is the primary action now; "Full breakdown" is a quiet link. Also
dropped the canned "My OpenStrap workout" caption, which is exactly the
filler that makes a share feel automated.
Reachable from the workout DETAIL screen too — it was finish-screen only,
so leaving that screen made a workout unshareable forever. Both go through
buildWorkoutShareData so the same run cannot produce two different cards;
there is a test asserting exactly that. A live workout is not shareable
(no final numbers yet).
Includes a real release-only bug I shipped and had to fix: the preview
called RenderRepaintBoundary.debugNeedsPaint, which is
bool get debugNeedsPaint {
late bool result;
assert(() { result = _needsPaint; return true; }());
return result;
}
Asserts are stripped in release/profile, so reading it throws
LateInitializationError and sharing worked in debug while failing on every
real build. Nothing catches this: the analyzer is happy and the whole test
suite runs in debug. test/no_debug_only_apis_test.dart now greps lib/ for
every getter in the Flutter SDK with that shape — the denylist was
derived, not guessed, and the regeneration command is in the file.
New OsIcon.share maps to FluentIcons.share_24_regular from a pack already
imported; goes through the OsIcon seam like every other icon.
|
Warning Review limit reached
Next review available in: 8 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR updates live workout lifecycle handling, location and screen-wake behavior, session layouts, route maps, workout sharing, design-system components, timeline presentation, privacy documentation, and regression/widget coverage. ChangesWorkout lifecycle and platform controls
Live session layout and rendering
Workout sharing
Design-system redesign
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant AppState
participant ScreenWake
participant DeriveScheduler
participant LiveSessionScreen
participant WorkoutSharePreviewScreen
AppState->>ScreenWake: enable during workout
AppState->>DeriveScheduler: hold derivation during workout
LiveSessionScreen->>WorkoutSharePreviewScreen: open composed share preview
WorkoutSharePreviewScreen->>WorkoutSharePreviewScreen: capture card and share PNG
AppState->>ScreenWake: release during teardown
AppState->>DeriveScheduler: resume derivation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Reviewer Guide 🔍(Review updated until commit 78cd72a)Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Latest suggestions up to 78cd72a Explore these optional code suggestions:
Previous suggestionsSuggestions up to commit 1e8cac2
Suggestions up to commit ea5bae6
Suggestions up to commit 062884a
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/absent_not_zero_test.dart (1)
441-453: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThis test asserts its own inputs, not
MiniBars' behaviour.
t.widget<MiniBars>(...).valuesjust reads back the list passed to the constructor, so the assertions hold no matter howMiniBarsrenders — the stated invariant (a null day keeps its slot rather than shifting the week left) is no longer guarded. This file already has_paintedBars(t); counting painted bars actually exercises it.💚 Assert the rendered output instead
await t.pump(const Duration(milliseconds: 700)); - final bars = t.widget<MiniBars>(find.byType(MiniBars)); - expect(bars.values.length, 7); // was 6 — Thu–Sun slid onto Wed–Sat - expect(bars.values[2], isNull); + // 7 slots, 6 drawn bars: the gap is skipped, not compacted away. + expect(_paintedBars(t), 6); expect(t.takeException(), isNull);As per coding guidelines, "Behavior changes, especially regressions involving readiness, abstention, idempotence, synchronization, migrations, and lifecycle safety, must include regression tests."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/absent_not_zero_test.dart` around lines 441 - 453, Update the “keeps a missing day in place” test to assert rendered output rather than reading the constructor input through bars.values. Use the existing _paintedBars(t) helper to verify the rendered bar count and preserve the null-day slot invariant, while retaining the exception check.Source: Coding guidelines
🧹 Nitpick comments (3)
test/design_redesign_test.dart (1)
63-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider also tapping the chip to prove it doesn't swallow the ring's
onTap.In
OrbitScorethe chip sits inside thePressablethat carriesonTap, andStateChipViewwraps itself in its ownPressable(with a null callback). The current test only taps the score text, so the chip-area tap path is unverified.💚 Extra assertion
await t.tap(find.text('82')); await t.pump(const Duration(milliseconds: 250)); expect(core, 1); + // A tap on the status chip must still reach the ring — the chip's own + // Pressable (no onTap) must not absorb it. + await t.tap(find.text('Push')); + await t.pump(const Duration(milliseconds: 250)); + expect(core, 2);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/design_redesign_test.dart` around lines 63 - 90, Extend the existing OrbitScore widget test around the StateChipView assertion to tap the rendered “Push” chip and verify the parent onTap still increments core. Keep the existing score-text tap assertion, and ensure the chip tap confirms StateChipView does not swallow OrbitScore’s Pressable callback.lib/ui/workouts/workouts_screen.dart (1)
1058-1088: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueShare data is formatted once at load, so a units change afterwards is not reflected.
_publishShareDatasnapshotsUnitsControlleroutput; switching metric/imperial while this screen is open leaves the share card on the old unit. Rebuilding the notifier value indidChangeDependencies(or storing raw values and formatting in_share) would keep it honest.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/ui/workouts/workouts_screen.dart` around lines 1058 - 1088, Update the workout share-data flow around _publishShareData so it is rebuilt when dependencies change, including after a UnitsController metric/imperial switch. Invoke _publishShareData from didChangeDependencies (while preserving the existing null, live-workout, and formatting behavior) so widget.shareData reflects the current units.lib/ui/activity/live_session_screen.dart (1)
1731-1742: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale comment block: it describes a banner that no longer exists.
The "Recording state" / "mutually exclusive with the stall banner" paragraph now sits directly above the re-centre button, with no recording banner in between — it reads as documentation for code that was deleted. Same pattern at Lines 1353-1369, where
_share's doc paragraph is duplicated almost verbatim.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/ui/activity/live_session_screen.dart` around lines 1731 - 1742, Remove the stale “Recording state” and “mutually exclusive with the stall banner” comments above the re-centre button, since no recording banner remains there. Also remove the duplicated obsolete _share documentation block around the corresponding earlier section, preserving only comments that describe currently existing code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/privacy.html`:
- Around line 87-91: Update the privacy statement in the route-related list item
to clarify that the app does not upload routes itself, while explicitly noting
that route imagery or data may leave the device when the user chooses to share a
workout. Preserve the existing claims about diagnostics and AI Coach access
where applicable.
In `@lib/compute/derive_scheduler.dart`:
- Around line 165-168: Update _drain() to re-check the workout/activity gate
immediately after await LocalDb.takeNextComputeJob() returns, before running
derivation. If a workout starts during acquisition, atomically requeue or retain
the claimed job and exit without processing it; alternatively serialize this
admission with workout-state changes, preserving existing behavior when no
workout is active.
In `@lib/gps/screen_wake.dart`:
- Around line 44-56: Update _set so _on is changed only after the platform
invokeMethod call completes successfully and returns true; do not latch the
requested value before the call. Treat Android’s false result, exceptions, and
unsuccessful release identically by leaving _on unchanged so later requests can
retry, while preserving the existing platform selection and error logging.
In `@lib/state/app_state.dart`:
- Around line 3307-3308: Move or add ScreenWake.enable() to the startWorkout()
flow and the orphan-session rehydration path so every live-workout entry,
including other, denied-location, and resumed non-route sessions, enables the
screen wake capability. Keep the existing unconditional ScreenWake.release()
teardown behavior unchanged.
- Line 754: Override notifyListeners() in the app state class to return without
notifying when _disposed is true, and otherwise delegate to the superclass
implementation. Keep setting _disposed in dispose() before super.dispose() so
scheduler callbacks and in-flight _afterDrain() continuations are covered by the
centralized guard.
In `@lib/ui/activity/live_session_screen.dart`:
- Around line 415-419: Update the almostText expression in the live session
screen to use Dart string interpolation for gapBpm and _zones[zone + 1].label
instead of escaping the dollar signs, so athletes see the calculated BPM gap and
zone label rather than the raw template.
In `@lib/ui/activity/workout_share_card.dart`:
- Around line 376-393: Update the share flow around the temporary PNG creation
to reuse a stable filename or remove prior openstrap_*.png files before writing,
and delete the generated file after sharing completes when safe. In the catch
block, log the exception through the existing logging mechanism and replace the
interpolated “Couldn’t share: $e” snackbar text with a fixed user-facing
message.
- Around line 547-562: Update the route handling around hasRoute so Dart
promotes the nullable value before use: bind route to a local non-null variable
and guard it with a direct null check plus hasPath, then use that promoted
variable for distanceMeters and points within the block.
In `@lib/ui/kit/route_map.dart`:
- Around line 185-194: Update _rebuildIfNeeded so its memoization key also
tracks the active palette and widget.interactive, rebuilding _points, _glow, and
_crisp whenever either changes even when widget.vertices is identical. Preserve
the existing identity-based vertex optimization and update all cached key state
after a rebuild.
In `@test/workout_reliability_test.dart`:
- Around line 81-89: Strengthen the live-workout test by enqueueing a light or
heavy derivation job before waiting past the settle interval, then assert it
remains unrun while the workout is active. Release the workout hold afterward
and assert the queued job runs exactly once, using the existing scheduler/job
APIs and the test’s runs counter.
- Around line 92-150: Update the ScreenWake test setup and implementation seam
so enable/release dispatch can be exercised on host tests without relying on
Platform.isAndroid or Platform.isIOS; ensure the repeated-enable call-count and
channel-failure tests invoke the mocked MethodChannels, while preserving
production platform gating.
In `@test/workout_share_card_test.dart`:
- Around line 21-27: Update the zone argument in the _route() helper to convert
the result of (i ~/ 8).clamp(0, 5) to an int before passing it to RouteVertex,
while preserving the existing clamping range and values.
---
Outside diff comments:
In `@test/absent_not_zero_test.dart`:
- Around line 441-453: Update the “keeps a missing day in place” test to assert
rendered output rather than reading the constructor input through bars.values.
Use the existing _paintedBars(t) helper to verify the rendered bar count and
preserve the null-day slot invariant, while retaining the exception check.
---
Nitpick comments:
In `@lib/ui/activity/live_session_screen.dart`:
- Around line 1731-1742: Remove the stale “Recording state” and “mutually
exclusive with the stall banner” comments above the re-centre button, since no
recording banner remains there. Also remove the duplicated obsolete _share
documentation block around the corresponding earlier section, preserving only
comments that describe currently existing code.
In `@lib/ui/workouts/workouts_screen.dart`:
- Around line 1058-1088: Update the workout share-data flow around
_publishShareData so it is rebuilt when dependencies change, including after a
UnitsController metric/imperial switch. Invoke _publishShareData from
didChangeDependencies (while preserving the existing null, live-workout, and
formatting behavior) so widget.shareData reflects the current units.
In `@test/design_redesign_test.dart`:
- Around line 63-90: Extend the existing OrbitScore widget test around the
StateChipView assertion to tap the rendered “Push” chip and verify the parent
onTap still increments core. Keep the existing score-text tap assertion, and
ensure the chip tap confirms StateChipView does not swallow OrbitScore’s
Pressable callback.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c45b3aff-a793-4e6a-8728-5e4590fb3a1b
📒 Files selected for processing (41)
PRIVACY.mdandroid/app/src/main/kotlin/wtf/openstrap/openstrap_edge/EdgeTrackingService.ktandroid/app/src/main/kotlin/wtf/openstrap/openstrap_edge/NativeChannels.ktdocs/privacy.htmlios/Runner/AppDelegate.swiftios/Runner/Info.plistlib/ai/briefing_engine.dartlib/compute/derive_scheduler.dartlib/gps/gps_source.dartlib/gps/screen_wake.dartlib/main.dartlib/state/app_state.dartlib/theme/tokens.dartlib/ui/activity/live_session_screen.dartlib/ui/activity/workout_share_card.dartlib/ui/design/design.dartlib/ui/design/gallery_screen.dartlib/ui/design/nav_pill.dartlib/ui/design/orbit_score.dartlib/ui/design/radial_heatmap.dartlib/ui/design/recap_card.dartlib/ui/design/state_chips.dartlib/ui/kit/charts.dartlib/ui/kit/kit.dartlib/ui/kit/os_icons.dartlib/ui/kit/route_map.dartlib/ui/timeline/timeline_screen.dartlib/ui/today/today_screen.dartlib/ui/workouts/workouts_screen.darttest/absent_not_zero_test.darttest/ai_briefing_test.darttest/core_screens_test.darttest/design_redesign_test.darttest/design_system_test.darttest/live_session_layout_test.darttest/no_debug_only_apis_test.darttest/ui_kit_new_widgets_test.darttest/workout_reliability_test.darttest/workout_share_card_test.darttest/workout_sleep_redesign_test.darttest/zone_contrast_test.dart
💤 Files with no reviewable changes (5)
- lib/ui/kit/kit.dart
- lib/ui/design/radial_heatmap.dart
- lib/ui/design/nav_pill.dart
- lib/ui/design/design.dart
- test/ui_kit_new_widgets_test.dart
| final hasRoute = route != null && route.hasPath; | ||
| final title = type.isEmpty | ||
| ? 'Workout' | ||
| : type[0].toUpperCase() + type.substring(1); | ||
|
|
||
| String heroValue; | ||
| String heroUnit; | ||
| List<(String, String)> stats; | ||
| if (hasRoute) { | ||
| final parts = units.distance(route.distanceMeters).split(' '); | ||
| heroValue = parts.first; | ||
| heroUnit = parts.length > 1 ? parts.sublist(1).join(' ') : ''; | ||
| stats = [ | ||
| (_shareDuration(duration), 'Time'), | ||
| // Moving pace, like everywhere else — see the note in _GpsControlPanel. | ||
| (units.pace(route.distanceMeters, route.movingSec), 'Pace'), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Dart type promotion via local boolean variable nullable parameter supported version
💡 Result:
Dart's flow-based type promotion supports local variables (including nullable parameters) when they are checked against null or a specific type within a function [1][2][3]. Regarding your query about boolean variables and type promotion: Dart does not perform "type predicates" like TypeScript, where a boolean function return value automatically triggers type promotion in the calling scope [4]. However, Dart's flow analysis does track promotions associated with boolean expressions within the same function body [4]. Key details include: 1. Scope Limitation: Type promotion occurs based on control flow analysis and is strictly limited to the current function scope [4]. Dart cannot promote a variable based on the result of a separate function call or an external boolean variable (i.e., promotion does not "escape" the scope where the check occurred) [4]. 2. Supported Targets: As of Dart 3.2, type promotion applies to local variables, parameters, and private final fields [1][5]. Previously, only local variables were promotable [1][5]. 3. Mechanism: If you need to promote a nullable parameter (e.g., String? x) or a property, the recommended approach is to assign the value to a local final variable at the beginning of the scope (e.g., final localX = x;) and perform your checks on that local variable [6][5]. Because local variables cannot be modified by external code or overridden, Dart's flow analysis can soundly promote them [7][5]. In summary, while you cannot use a custom boolean flag to "activate" promotion across different scopes, you can use local variables and standard null checks (if (x != null)) to achieve type promotion effectively within a single function [6][1][5].
Citations:
- 1: https://dart.dev/null-safety/understanding-null-safety
- 2: https://github.com/dart-lang/language/blob/master/resources/type-system/flow-analysis.md
- 3: https://github.com/dart-lang/language/blob/7eeb67b0d29b696b3c3ec8f9fe322334a2d5d87a/accepted/future-releases/nnbd/feature-specification.md
- 4: https://stackoverflow.com/questions/79171040/is-there-a-feature-in-dart-to-cause-variables-type-promotion-after-a-function-c
- 5: https://github.com/dart-lang/site-www/blob/main/src/content/tools/non-promotion-reasons.md
- 6: https://dart.dev/tools/non-promotion-reasons
- 7: https://stackoverflow.com/questions/76014276/dart-type-promotion-why-can-only-local-variables-be-promoted
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the file around the referenced lines.
FILE="lib/ui/activity/workout_share_card.dart"
wc -l "$FILE"
sed -n '500,620p' "$FILE"
# Find the declaration of `route` and `hasRoute` in this file.
rg -n "\broute\b|\bhasRoute\b" "$FILE"Repository: OpenStrap/edge
Length of output: 4641
🌐 Web query:
Dart promotion through final bool local variable if (hasRoute) route.distanceMeters
💡 Result:
In Dart, you cannot directly use a local boolean variable to "promote" a separate variable (such as route) to a non-nullable type within an if block [1][2]. Even if the boolean variable (e.g., final bool hasRoute = route != null;) is declared as final, Dart's flow analysis engine does not automatically link the state of that boolean to the nullability of the original variable route [1][2]. The compiler treats these as independent entities; it does not track that the truth value of the boolean implies the nullability status of the other variable [2]. To achieve type promotion, you must perform the null check directly within the if statement itself [3][2]. Recommended patterns: 1. Direct check (Preferred): Perform the null check directly in the condition. This allows Dart to soundly promote the variable for the scope of the block [3][4]. dart if (route != null) { // route is promoted to non-nullable here print(route.distanceMeters); } 2. Local variable assignment (Alternative): If you must use a local variable, assign the nullable object to a new, non-nullable local variable inside the block. This is a common and safe pattern [3][4]. dart final localRoute = route; if (localRoute != null) { // Use localRoute instead of the original route print(localRoute.distanceMeters); } While there have been discussions and proposals regarding more advanced flow analysis features, the current language design requires the check to be tied directly to the variable being promoted [5][2]. Using an intermediate boolean variable will generally result in a compilation error (e.g., "The property can't be accessed because the receiver can be null") because the compiler cannot guarantee the relationship between the two variables [1][2].
Citations:
- 1: Boolean variables sometimes fail to promote when implicitly typed dart-lang/language#1785
- 2: https://stackoverflow.com/questions/67735992/why-flow-analysis-doesnt-work-when-using-bool
- 3: https://dart.dev/tools/non-promotion-reasons
- 4: https://dart.dev/effective-dart/usage
- 5: Local variable is not promoted when checked that its value is a non-null literal dart-lang/language#1726
Use a direct null check here. hasRoute does not promote route, so route.distanceMeters and route.points are still nullable inside this block. Bind final r = route; and check if (r != null && r.hasPath) (or check route != null directly) before using it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/ui/activity/workout_share_card.dart` around lines 547 - 562, Update the
route handling around hasRoute so Dart promotes the nullable value before use:
bind route to a local non-null variable and guard it with a direct null check
plus hasPath, then use that promoted variable for distanceMeters and points
within the block.
| List<RouteVertex> _route() => [ | ||
| for (var i = 0; i < 40; i++) | ||
| RouteVertex( | ||
| LatLng(51.5074 + i * 0.0004, -0.1278 + i * 0.0003), | ||
| (i ~/ 8).clamp(0, 5), | ||
| ), | ||
| ]; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/sh
rg -n -A20 -B2 'class RouteVertex|const RouteVertex|RouteVertex\(' lib/gps/route_models.dart
rg -n '\.clamp\(' test/workout_share_card_test.dartRepository: OpenStrap/edge
Length of output: 1480
Convert the clamped zone to int. int.clamp() returns num, so this won’t type-check against RouteVertex’s int? zone.
Proposed fix
- (i ~/ 8).clamp(0, 5),
+ ((i ~/ 8).clamp(0, 5)).toInt(),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| List<RouteVertex> _route() => [ | |
| for (var i = 0; i < 40; i++) | |
| RouteVertex( | |
| LatLng(51.5074 + i * 0.0004, -0.1278 + i * 0.0003), | |
| (i ~/ 8).clamp(0, 5), | |
| ), | |
| ]; | |
| List<RouteVertex> _route() => [ | |
| for (var i = 0; i < 40; i++) | |
| RouteVertex( | |
| LatLng(51.5074 + i * 0.0004, -0.1278 + i * 0.0003), | |
| ((i ~/ 8).clamp(0, 5)).toInt(), | |
| ), | |
| ]; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/workout_share_card_test.dart` around lines 21 - 27, Update the zone
argument in the _route() helper to convert the result of (i ~/ 8).clamp(0, 5) to
an int before passing it to RouteVertex, while preserving the existing clamping
range and values.
CodeRabbit found 12 things on #162. Ten were valid; two were wrong and are left alone with the reasoning recorded below. CRITICAL, and mine: the almost-there nudge shipped as '\$gapBpm bpm to \${...} — push' — escaped, so the athlete saw the raw template instead of "4 bpm to Z4 — push". A shell/python escape leaked into Dart source. No test covered it because the nudge only appears within 5 bpm of the next zone; there is one now. Two of my tests asserted nothing, which is the same failure I caught in my own layout test earlier and did not generalise from: - the derive-gate test never enqueued a job, so `runs == 0` passed with the gate deleted. It now queues real work, asserts it stays parked, and asserts it drains on release. Verified by deleting the gate. - the ScreenWake tests mocked two MethodChannels that were never reached: Platform.isAndroid and isIOS are BOTH false on the host VM, so the dispatch short-circuited. Added a test seam. Correctness: - _drain() cleared the gate, then awaited takeNextComputeJob(); a workout starting inside that window still got a derive pass, and the job was already marked running. Re-checks after acquisition and hands the job back via a new LocalDb.requeueComputeJob (which also undoes the attempt increment — being deferred is not a failure). - ScreenWake latched _on before the platform answered. Android returns false when no activity is attached, so a failed enable left Dart believing the screen was held and suppressed every later retry. - _disposed only guarded the paths someone remembered. notifyListeners() is now overridden to no-op when disposed, which covers the scheduler's onChanged callback and in-flight _afterDrain() continuations too. - ScreenWake was armed inside _maybeStartRouteTracking, so indoor workouts, location-denied runs and resumed non-route sessions never held the screen. Moved to startWorkout() plus orphan rehydration. - RouteMapView's memo key was the vertex list alone, but the cached polylines bake in the ACTIVE palette (via _colorFor) and `interactive` (stroke width). A theme switch left a finished route in the old palette's colours — and the design gallery toggles the theme with this widget on screen. Key now includes both, and is re-checked in build() because the palette is a global static that need not trigger didUpdateWidget. Privacy: "Routes never leave your device" was too absolute now that Share renders a route into an image the user can send anywhere. Split into what the App does (never sends them) and the one exception (you, deliberately, having seen the image first). Both documents. Also: shared PNGs used a timestamped filename and accumulated in temp until the OS reclaimed them — one reused name now. And "Couldn't share: $e" put an internal exception string in front of the athlete; logged instead, with a fixed sentence shown. REJECTED, with reasons: - "hasRoute does not promote route, so route.distanceMeters is still nullable": Dart 3.11 DOES promote through a final boolean local. Verified with a standalone probe; the analyzer is clean. - "int.clamp() returns num, so this won't type-check against int?": int.clamp(int, int) is statically int in Dart. Also verified with a probe that analyzes clean. Unrelated flake found while verifying: notification_day_guard_test seeded empty prefs for the cases that expect a present to SUCCEED, inheriting the default 22:00-07:00 quiet window. Every one of those failed for nine hours a night locally and passed on CI only because CI runs at a different hour. Pinned quiet hours off, mirroring the _quietAllDay helper already there. 959 tests, analyzer clean.
Review triage — 10 fixed, 2 disprovedThanks, these were good. Verified every finding against the code rather than taking them on trust; two didn't survive that. The important one
Two of my tests asserted nothingBoth correctly called out:
Correctness
PrivacyRight that "Routes never leave your device" was too absolute now that Share renders a route into a sendable image. Split into what the App does (never sends them) and the one exception — you, deliberately, having seen the image first. Both documents. Also fixed: shared PNGs used a timestamped filename and accumulated in temp; and Not appliedBoth disproved with standalone probes that analyze clean:
Unrelated flake, found while verifying
959 tests, analyzer clean. |
|
Persistent review updated to latest commit ea5bae6 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/state/app_state.dart`:
- Around line 3227-3231: Serialize all ScreenWake transitions through one shared
operation so enable/release calls cannot overtake each other: update the start
and rehydration paths in lib/state/app_state.dart at lines 3227-3231 and 3436,
and serialize stop and cancellation teardown at lines 3465-3470 and 3534-3535,
ensuring teardown waits for any pending enable. In
test/workout_reliability_test.dart lines 123-191, add concurrent enable/release
coverage and verify ordered true-then-false channel calls with a final unheld
state.
In `@test/workout_reliability_test.dart`:
- Around line 81-97: The test around the queued-job session flow must exercise
the post-claim requeue path rather than only enqueueing while the gate is
closed. Arrange for the job to be acquired, then activate the workout before
execution completes; assert it is returned to queued with its attempt count
unchanged, and verify it executes exactly once after the workout is released.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 19c9004e-c898-4537-9204-7029563d1538
📒 Files selected for processing (12)
PRIVACY.mddocs/privacy.htmllib/compute/derive_scheduler.dartlib/data/db.dartlib/gps/screen_wake.dartlib/state/app_state.dartlib/ui/activity/live_session_screen.dartlib/ui/activity/workout_share_card.dartlib/ui/kit/route_map.darttest/live_session_layout_test.darttest/notification_day_guard_test.darttest/workout_reliability_test.dart
🚧 Files skipped from review as they are similar to previous changes (7)
- docs/privacy.html
- lib/gps/screen_wake.dart
- test/live_session_layout_test.dart
- lib/compute/derive_scheduler.dart
- lib/ui/kit/route_map.dart
- lib/ui/activity/workout_share_card.dart
- lib/ui/activity/live_session_screen.dart
| // Hold the display for EVERY live session, not just route-eligible ones. | ||
| // Arming this from _maybeStartRouteTracking meant an indoor workout, a | ||
| // location-denied run, and a resumed non-route session all watched the | ||
| // screen sleep mid-set. Released unconditionally on both teardown paths. | ||
| ScreenWake.enable(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Serialize ScreenWake transitions.
enable() and release() inspect _on before their platform await. Thus a stop can skip release() while enable is in flight, after which enable latches on; conversely, a rapid new start can skip enable while release is in flight.
lib/state/app_state.dart#L3227-L3231: enqueue the start transition through a serialized ScreenWake operation.lib/state/app_state.dart#L3436-L3436: use the same serialized transition on rehydration.lib/state/app_state.dart#L3465-L3470: serialize stop after any pending enable.lib/state/app_state.dart#L3534-L3535: serialize cancellation teardown after any pending enable.test/workout_reliability_test.dart#L123-L191: addFuture.wait([ScreenWake.enable(), ScreenWake.release()])coverage and assert the final state is unheld with orderedtrue, thenfalsechannel calls.
Proposed ScreenWake fix
+ static Future<void> _transition = Future.value();
+
static Future<void> _set(bool on) async {
+ _transition = _transition.then((_) => _setOnce(on));
+ return _transition;
+ }
+
+ static Future<void> _setOnce(bool on) async {
if (on == _on) return;
try {
...As per coding guidelines, lib/**/*.dart: “When adding or changing a capability, cover every call path”; and test/**/*.dart: “Behavior changes … and lifecycle safety, must include regression tests.”
📍 Affects 2 files
lib/state/app_state.dart#L3227-L3231(this comment)lib/state/app_state.dart#L3436-L3436lib/state/app_state.dart#L3465-L3470lib/state/app_state.dart#L3534-L3535test/workout_reliability_test.dart#L123-L191
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/state/app_state.dart` around lines 3227 - 3231, Serialize all ScreenWake
transitions through one shared operation so enable/release calls cannot overtake
each other: update the start and rehydration paths in lib/state/app_state.dart
at lines 3227-3231 and 3436, and serialize stop and cancellation teardown at
lines 3465-3470 and 3534-3535, ensuring teardown waits for any pending enable.
In test/workout_reliability_test.dart lines 123-191, add concurrent
enable/release coverage and verify ordered true-then-false channel calls with a
final unheld state.
Source: Coding guidelines
CI caught a flake I introduced one commit ago: "a queued job stays parked for the session, then runs on release" failed with Expected: <1>, Actual: <0>. The drain does several DB round-trips and a fixed 120 ms sleep is simply not enough headroom on a loaded runner — it passed on this machine every time and failed on GitHub's. Polls for the positive direction with a 5 s ceiling, returning as soon as the condition holds, so the generous timeout costs nothing locally. The negative direction keeps a fixed wait, because you cannot poll for "this never happens"; 150 ms is comfortably past the 10 ms settle, and a longer wait there only makes that assertion stronger. Re-verified the test still bites by deleting the gate. Worth noting the first attempt at that check was itself wrong: sed only removed the _arm/_drain entry condition and the test still passed, because the post-acquisition re-check added for the CodeRabbit race is a genuine SECOND gate. Removing both does fail the test.
|
Persistent review updated to latest commit 1e8cac2 |
Second CodeRabbit pass, both findings valid. The race: _on is only updated AFTER the platform await, so a release() arriving while an enable() is still in flight read the stale `false`, decided it had nothing to do, and returned — then the in-flight enable latched _on = true and the display stayed held for the rest of the app's life. Both call sites in AppState are fire-and-forget, so starting a workout and immediately stopping it was enough to hit it. Transitions now run through a chain, so each sees the state the previous one left. Covered by a test that fails on the unserialized version (isHeld true instead of false) and asserts both transitions reach the platform in order. Also adds the requeueComputeJob coverage that was missing: the gate test closed the gate BEFORE enqueueing, so no job was ever claimed and the requeue path never ran. That test deliberately exercises the PRIMITIVE rather than simulating the interleaving. Hitting the real window means racing a DB round-trip, which is a coin flip dressed up as a test — this same file already shipped one of those and it failed on CI. What is pinned instead is the guarantee the drain path depends on: a claimed job is not handed out twice, comes back claimable after a requeue, and a deferral does not burn an attempt. Verified by removing the attempts decrement and watching it fail. One wart found while writing it, left alone: takeNextComputeJob returns the row as it was BEFORE its own update, so the `attempts` it reports is the pre-increment value. Documented in the test rather than changed — nothing depends on that field today and it is not this PR's business. 962 tests, analyzer clean.
|
Persistent review updated to latest commit 78cd72a |
1 participant
User description
Five commits, each reviewable on its own. The privacy one (
10afbe3) wants your eyes specifically — it changes a user-facing promise.Started from "the activity screen looks bad" and "the app closes in the middle of a ride", and both turned out to be structural rather than cosmetic.
8a7825f— prune the design system to what actually shipsScanned every public type in
ui/design+ui/kitfor call sites outside its own file and the gallery, then deleted what had none:RadialHeatmap,RecapCard,AreaSpark,DotMatrix,CalendarHeatmap,StatTile,BaselineProgress,NightCard,NavPillAction,StateChips, OrbitScore's satellite layer, and the deadTimelineScreenwrapper.Where a deleted component carried a real invariant, the test moved rather than went — RecapCard's "a missing day holds its slot" now runs against
MiniBars, whose invariant it actually was.−1060 lines.
f5cda0b— stop losing runs and rides mid-sessionFive independent causes, each separately sufficient. The two least obvious:
EdgeTrackingService.start(Context)sends noEXTRA_LOCATION, defaulting tofalse— andCompanionBridge.onDeviceAppearedcalls it whenever the band re-enters BLE range, which happens routinely mid-run from arm-swing dropouts. That re-startForeground'd asconnectedDeviceonly. The Dart-side sticky flag could never protect it; these callers never go through Dart.10afbe3— background route recordingWithout the
locationbackground mode iOS suspends within seconds of a screen lock. Scope is tight and every claim was verified, not assumed:geolocator_apple's handler is anif/else if, and because we shipNSLocationWhenInUseUsageDescriptionthe Always branch is unreachable_settings()→ one caller → one caller, torn down on every workout-end pathworkout_routetableThe old string said "OpenStrap does not track your location in the background" — false the moment this ships, so it's rewritten. Both privacy docs gain a Location and workout routes section; neither mentioned location at all, which was already a gap.
No store privacy-label change needed. Apple's "collect" means transmission off device and explicitly excludes on-device-only processing. Android uses
FOREGROUND_SERVICE_LOCATION, notACCESS_BACKGROUND_LOCATION, so no Play declaration is triggered. Guideline 2.5.4 is the thing to satisfy, via the purpose strings.15d3f2e— rebuild the live session screenThe overlaps were structural: one flat
Stackof absolutely-positioned layers with no relationship between them. The re-centre button was pinnedbottom: 96while the panel is far taller, so it rendered underneath it. Now a bounded hero and a metric sheet as siblings in a Column — overlap is impossible rather than unlikely.Bugs fixed alongside, all reported from real use:
40:32 /kmafter barely movingTileLayer.maxZoommeans "draw nothing above this"; wantedmaxNativeZoomcool, a surface token, and Z0 is the resting zoneColors.white30= 2.72:1 where small text needs 4.5:1Contrast was measured, not eyeballed, and
zone_contrast_test.dartguards all six zones at ≥3:1.062884a— a real share cardSharing rasterised the entire finish card into one tall PNG and handed it to the OS sheet unseen. Now composed for the job: map full-bleed, one headline figure, three stats, nothing else, with a preview before send. Reachable from the workout detail screen too — it was finish-screen only, so leaving that screen made a workout unshareable forever.
Includes a release-only bug I shipped and had to fix: the preview called
debugNeedsPaint, whose value is only assigned inside anassert. Stripped in release ⇒LateInitializationError⇒ sharing worked in debug and failed on every real build.no_debug_only_apis_test.dartnow greps for every SDK getter with that shape (denylist derived from the SDK, not guessed).Verification
flutter test --concurrency=1), up from 914flutter analyzecleanflutter build apk --release— release specifically, because thedebugNeedsPaintclass of bug is invisible in debugflutter build ios --debug --no-codesignRegression tests were checked by deliberately reintroducing each bug and confirming they fail. That caught one of my own tests being vacuous: it asserted on the session clock, which clears the sheet even when broken. Rewritten against the bottom-most hero element; it now fails by 15.5 px.
PR Type
Enhancement, Bug fix
Description
Rebuilt live-session screen layout (Column hero+sheet) fixing overlapping controls, zone colour bugs, and battery-wasting animations on map view
Added
workout_share_card.dartwith a dedicated share-card composer and preview screen, replacing the raw screenshot approachFixed mid-session workout loss: Android FGS type stripping, milestone re-fire on screen return, moving-pace vs elapsed-pace, and derive scheduler now defers heavy compute during live workouts
Pruned dead design-system components (−1060 lines) and added splits/route-stat rows to the finish screen
Diagram Walkthrough
File Walkthrough
6 files
Full layout rebuild: Column hero+sheet, zone colour fix, share cardNew dedicated share-card composer and preview screenNew screen-wake helper for background route recording sessionsRoute map widget updates for new layout and stat bar changesStateChipView pill replaces loose text on Today readiness ringAdd zoneOnDark colour ramp for dark-background zone rendering3 files
Defer heavy derivation while a live workout is runningWire workout-active flag into derive scheduler and share flowFix Android FGS type stripping mid-session on BLE reconnect4 files
Remove dead ornamental chart widgets, update header commentRemove unused standalone TimelineScreen wrapper widgetRemove unused satellite layer from OrbitScorePrune gallery to only cover shipped components5 files
Tests for share card data composition and edge casesTests for live session layout correctness and milestone dedupTests for mid-session workout reliability fixesTest that debug-only APIs are not called in production pathsTests that zone colours meet contrast on dark background1 files
Add location background mode for iOS route recording1 files
Update privacy policy for background location usage21 files
Summary by CodeRabbit
Summary by CodeRabbit