-
-
Notifications
You must be signed in to change notification settings - Fork 83
fix: activity-type edit regression + Active Energy workout-gap credit #294
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
97f2e4b
e44502e
1746731
72d9228
1e7bd4a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,8 +29,10 @@ import '../grammar.dart'; | |
| import '../paint_activity.dart'; | ||
| import '../profile/profile.dart'; | ||
| import '../screens/home_screen.dart' show unitsOf; | ||
| import '../screens/log_workout.dart' show bumpInsights; | ||
| import '../theme.dart'; | ||
| import 'catalogue.dart'; | ||
| import 'picker.dart'; | ||
| // The share card and this screen describe the same session, so they draw its | ||
| // stats with the same widget and split its values with the same function. | ||
| // poster.dart imports this file back for [ActivityResult]; that is the seam, | ||
|
|
@@ -811,6 +813,45 @@ class _ActivitySummaryState extends State<ActivitySummary> { | |
| ]), | ||
| ); | ||
|
|
||
| /// Correct a session's activity type — the band's own guess, or a hand-typed | ||
| /// one that was wrong. `LocalDb.setSessionType` is the narrow UPDATE this | ||
| /// reuses; it used to have no caller at all (lost in the ui2 rewrite along | ||
| /// with the screen that called it). | ||
| /// | ||
| /// Archetype-specific fields on [r] (sets, route, splits…) belong to the OLD | ||
| /// type and cannot be salvaged for the new one, so this does not try to | ||
| /// patch [r] in place — it hands back to whatever list pushed this screen, | ||
| /// which re-reads on the revision bump below. | ||
| Future<void> _changeType(BuildContext c) async { | ||
| final id = r.sessionId; | ||
| if (id == null) return; | ||
| // Only true once a pick actually landed — pressing back out of the picker | ||
| // must return to this screen, not fall through and pop it too. | ||
| var picked = false; | ||
| await Navigator.of(c).push(MaterialPageRoute( | ||
| builder: (_) => ActivityPicker(onPick: (pc, newActivity) async { | ||
| // The stored key everywhere else uses — `startWorkout(type: | ||
| // a.typeKey)` is the live path's own write. `a.name` here would still | ||
| // resolve through `activityByName`'s normalized lookup, but it would | ||
| // store a different string than every other producer of this column. | ||
| try { | ||
| await LocalDb.setSessionType(id, newActivity.typeKey); | ||
| } catch (_) { | ||
| // Same rule as `_saveRpe`: the row is unchanged, so leave the | ||
| // picker open rather than close it over a write that never | ||
| // happened — `onPick` is a `void Function`, so there is no caller | ||
| // to hand this failure back to. | ||
| return; | ||
| } | ||
| picked = true; | ||
| if (!pc.mounted) return; | ||
| bumpInsights(pc); | ||
| Navigator.of(pc).pop(); | ||
|
Comment on lines
+832
to
+849
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: #!/bin/bash
set -euo pipefail
# Verify how ActivityPicker invokes and handles the void onPick callback.
ast-grep outline lib/ui2/activity/picker.dart --items all
rg -n -C 8 'onPick|widget\.onPick' lib/ui2/activity/picker.dartRepository: OpenStrap/edge Length of output: 1673 Handle failures from 🤖 Prompt for AI Agents |
||
| }), | ||
| )); | ||
| if (c.mounted && picked) Navigator.of(c).pop(); | ||
| } | ||
|
sourcery-ai[bot] marked this conversation as resolved.
|
||
|
|
||
| Future<void> _retrySave() async { | ||
| if (_saving) return; | ||
| setState(() => _saving = true); | ||
|
|
@@ -832,6 +873,11 @@ class _ActivitySummaryState extends State<ActivitySummary> { | |
| Widget build(BuildContext c) { | ||
| final p = P.of(c); | ||
| _u = unitsOf(c); | ||
| // Only a saved session has an id to correct — a draft on screen because | ||
| // the write threw has nowhere to put it. Reserving the two-icon width | ||
| // for a row that only ever draws one icon would shove the title left on | ||
| // every unsaved-session summary for no reason. | ||
| final canChangeType = r.sessionId != null; | ||
|
Comment on lines
+876
to
+880
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- applicable conventions ---'
find /tmp/coderabbit-repo-knowledge/openstrap-edge-7422777d -path '*/conventions/*.md' -o -path '*/guidelines/*.md' 2>/dev/null | sort | while read -r f; do
case "$f" in
*/lib/*) cat "$f" ;;
esac
done
printf '%s\n' '--- summary.dart target and retry symbols ---'
sed -n '800,930p' lib/ui2/activity/summary.dart
printf '%s\n' '--- retry callback declarations and call sites ---'
rg -n -C 4 'onRetrySave|_retrySave|ActivitySummary\(' lib test 2>/dev/null | head -300Repository: OpenStrap/edge Length of output: 21775 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- live save and retry implementation ---'
sed -n '400,520p' lib/ui2/activity/live.dart
printf '%s\n' '--- ActivityResult definition and sessionId behavior ---'
rg -n -C 8 'class ActivityResult|ActivityResult\(|sessionId|retrySave' lib/ui2 test/ui2_activity_test.dart | head -350
printf '%s\n' '--- summary state fields and existing retry test ---'
sed -n '645,720p' lib/ui2/activity/summary.dart
sed -n '1380,1450p' test/ui2_activity_test.dartRepository: OpenStrap/edge Length of output: 35312 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- all onFinish providers and retry contract ---'
rg -n -C 10 'onFinish:|Future<ActivityResult>|ActivityResult Function|stopWorkout|save.*Session|startWorkout' lib/ui2 lib/data lib 2>/dev/null | head -400
printf '%s\n' '--- ActivityResult declaration ---'
rg -n -g '*.dart' 'class ActivityResult|typedef.*ActivityResult|sessionId:' lib | head -120Repository: OpenStrap/edge Length of output: 31240 Use the saved result after a successful retry.
🤖 Prompt for AI AgentsSource: Coding guidelines |
||
| return Scaffold( | ||
| backgroundColor: p.bg, | ||
| body: SafeArea( | ||
|
|
@@ -841,12 +887,27 @@ class _ActivitySummaryState extends State<ActivitySummary> { | |
| child: NavBar( | ||
| a.name, | ||
| sub: _shortDate(r.start).toUpperCase(), | ||
| trailing: Pressable( | ||
| semanticLabel: 'Share this ${a.name.toLowerCase()}', | ||
| onTap: () => Navigator.of(c).push(MaterialPageRoute( | ||
| builder: (_) => ShareSheet(r))), | ||
| child: Icon(LucideIcons.share2, size: 19, color: p.ink2), | ||
| ), | ||
| // Two icons, each a Pressable with S.tap's own 44 pt minimum | ||
| // hit box (grammar.dart's accessibility floor, not optional) — | ||
| // S.tap * 2 alone is 12 pt short of that plus the gap between | ||
| // them, which is exactly the RenderFlex overflow this fixed. | ||
| trailingWidth: canChangeType ? S.tap * 2 + S.x3 : S.tap, | ||
| trailing: Row(mainAxisSize: MainAxisSize.min, children: [ | ||
| if (canChangeType) ...[ | ||
| Pressable( | ||
| semanticLabel: 'Change activity type', | ||
| onTap: () => _changeType(c), | ||
| child: Icon(LucideIcons.pencil, size: 18, color: p.ink2), | ||
| ), | ||
| const SizedBox(width: S.x3), | ||
| ], | ||
| Pressable( | ||
| semanticLabel: 'Share this ${a.name.toLowerCase()}', | ||
| onTap: () => Navigator.of(c).push(MaterialPageRoute( | ||
| builder: (_) => ShareSheet(r))), | ||
| child: Icon(LucideIcons.share2, size: 19, color: p.ink2), | ||
| ), | ||
| ]), | ||
| ), | ||
| ), | ||
| Padding( | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Query sessions across the full attribution window.
Both session lookups use
LocalDb.sessionsInRange, which filters bystart_ts, but the bounds are derived from the first and last timestamps present indaySub. A workout beginning in a leading or trailing no-record gap can therefore be excluded before_hasHrCoverageruns, causing its zero-HR calories to be omitted from Active Energy. Use calendar-day bounds or include sessions overlapping the attribution window.📍 Affects 1 file
lib/compute/derivation_engine.dart#L5158-L5162(this comment)lib/compute/derivation_engine.dart#L7157-L7157🤖 Prompt for AI Agents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair catch but leaving this one — sessionsInRange's dayLo/dayHi already feeds workout detection and the HR ceiling calc elsewhere in the same function, widening it to calendar-day bounds just for the credit path risks changing those too. the gap only bites a workout that starts before the day's first record or after its last, which isn't the case this PR was written for. noted as a follow-up, not blocking this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abdulsaheel, understood. The shared session query supports workout detection and HR ceiling logic, so changing its bounds in this PR could expand the change scope.The missed session case remains valid but is outside the target behavior of
#294. A follow-up can add an attribution-specific overlapping-session query without changing the existing consumers.Would you like me to open a GitHub issue for that follow-up?
✏️ Learnings added
You are interacting with an AI system.