Feat/active workout protection - #39
Merged
Merged
Conversation
- Added a new WorkoutConflictDialog to manage conflicts when starting a workout. - Refactored workout starting logic in ProgramDetailScreen and RoutinesScreen to handle conflicts. - Introduced draft persistence for active workouts in WorkoutProvider, allowing restoration of workouts after app closure. - Enhanced workout flow management with options to discard or resume workouts. - Updated tests to cover new draft persistence and conflict handling features. Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Contributor
WalkthroughAdds active-workout draft persistence and single-workout enforcement to the provider; exposes a conflict-resolution API ( Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@workout-logger/lib/screens/programs/program_detail_screen.dart`:
- Around line 106-110: When handling the branch that currently does
Navigator.push(... builder: (_) => WorkoutFlowScreen(programDay: day,
programWeek: week)) for started || conflictAction ==
StartWorkoutConflictAction.resume, stop using the newly tapped day/week and
instead pass the active workout’s original program context (e.g. use
activeWorkout.programDay and activeWorkout.programWeek or the equivalent fields
from your workout state/store) into WorkoutFlowScreen so the resumed route uses
the existing workout’s structure; null-check the active workout and fall back to
the tapped day/week only if no active workout exists.
In `@workout-logger/lib/services/workout_provider.dart`:
- Around line 145-153: The draft JSON saved in workout_provider (built with
_draftSchemaVersion, _workoutStartTime, _activeRoutine?.id,
_currentExerciseIndex, and _currentExerciseLogs) lacks program-specific context
so WorkoutFlowScreen cannot rebuild deload targets, rest timings, supersets or
slot lookups from programDay/programWeek when resuming; update the persisted
draft to include the workout origin/slot metadata (e.g.,
programId/programWeek/programDay/slotId or a serialized origin enum) or
alternatively move the programDay/programWeek/slot state into the provider so it
is always serialized; ensure both the creation site that builds the draft and
the restore logic that reads it handle the new fields (also apply the same
change at the other draft persistence area noted around lines 212-231).
- Around line 135-160: _async draft saves from _persistDraft() can complete
after callers use unawaited(...) and then call finishWorkout() or
cancelWorkout() which calls _clearDraft(), causing a stale save to recreate a
cleared draft; fix by serializing/awaiting pending draft writes: introduce a
single tracking Future or version counter (e.g., _lastDraftWrite or
_draftWriteVersion) updated by _persistDraft() and awaited by _clearDraft() (and
by finishWorkout()/cancelWorkout() before they clear _draftKey), or have
_persistDraft() check the current version before writing to _storage.saveSetting
to avoid writing stale drafts; update references in addSet(), nextExercise(),
finishWorkout(), cancelWorkout(), _persistDraft(), _clearDraft() to use this
mechanism.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c622dce7-ae4b-4779-9ceb-c626cac2c31c
📒 Files selected for processing (9)
workout-logger/lib/screens/home_screen.dartworkout-logger/lib/screens/programs/program_detail_screen.dartworkout-logger/lib/screens/routines_screen.dartworkout-logger/lib/screens/widgets/workout_conflict_dialog.dartworkout-logger/lib/screens/workout_flow_screen.dartworkout-logger/lib/services/workout_provider.dartworkout-logger/test/performance_regression_test.dartworkout-logger/test/test_utils/mock_storage_service.dartworkout-logger/test/workout_provider_test.dart
💤 Files with no reviewable changes (1)
- workout-logger/test/performance_regression_test.dart
… conflict resolution
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
workout-logger/lib/screens/workout_flow_screen.dart (1)
218-235:⚠️ Potential issue | 🟡 MinorSystem-back bypasses the leave/cancel flow when in the exercise selector.
_buildExerciseSelector()(line 220) returns a plainScaffoldwithoutPopScope, so whencurrentExerciseLogs.isEmpty(quick-start, no exercises picked yet) a hardware/gesture back silently pops the route while the provider still hashasActiveWorkout == trueand a persisted (empty) draft. The explicit close button on this view goes through_showCancelDialog— the back gesture should be consistent. After re-launch the empty draft restores and the user re-enters the selector with the same dangling state.🛡️ Suggested fix — wrap the selector in the same PopScope
Widget _buildExerciseSelector() { - return Scaffold( + return PopScope( + canPop: false, + onPopInvokedWithResult: (didPop, _) { + if (didPop) return; + unawaited(_handleSystemBack()); + }, + child: Scaffold( appBar: AppBar( title: const Text('Select Exercises'), leading: IconButton( icon: const Icon(Icons.close), onPressed: _showCancelDialog, ), ), body: ExerciseSelectorScreen( selectionMode: true, onExercisesSelected: _startWithSelectedExercises, ), - ); + ), + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workout-logger/lib/screens/workout_flow_screen.dart` around lines 218 - 235, The exercise selector path bypasses the PopScope protection causing system-back to silently pop while hasActiveWorkout remains true; wrap the selector in the same PopScope (or move the existing PopScope so it encloses both branches) and reuse the same onPopInvokedWithResult behavior that calls _handleSystemBack when didPop is false so hardware/gesture back triggers the same cancel flow as the explicit close button; ensure you keep canPop: false and reference provider.currentExerciseLogs, _buildExerciseSelector, PopScope, and _handleSystemBack when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@workout-logger/lib/screens/workout_flow_screen.dart`:
- Around line 66-78: The current resolvers _resolvedProgramDay and
_resolvedProgramWeek prefer widget.programDay/Week over the provider’s active
values, which can render stale context when a live workout exists; change both
functions to prefer provider.activeProgramDay/activeProgramWeek whenever
provider.hasActiveWorkout (and that active value is non-null), falling back to
widget.programDay/Week only when there is no active session or the provider
value is null; ensure _slotForIndex (which calls _resolvedProgramDay and accepts
an optional provider) continues to pass through the resolved provider so lookups
use the new precedence.
In `@workout-logger/lib/services/workout_provider.dart`:
- Around line 188-275: In _restoreDraftIfAny validate the restored index against
the restoredLogs length: compute maxIndex = restoredLogs.isEmpty ? 0 :
restoredLogs.length - 1 and if restoredLogs.isNotEmpty and indexFromDraft is
outside 0..maxIndex then debugPrint a clear message and await _clearDraft() and
return (or alternatively log+clear), otherwise set _currentExerciseIndex =
indexFromDraft.clamp(0, maxIndex) (remove the redundant .toInt()); also remove
the redundant assignment _activeSession = null since it's already null. Ensure
you reference indexFromDraft, restoredLogs, maxIndex, _clearDraft,
_currentExerciseIndex, and _activeSession in the changes.
---
Outside diff comments:
In `@workout-logger/lib/screens/workout_flow_screen.dart`:
- Around line 218-235: The exercise selector path bypasses the PopScope
protection causing system-back to silently pop while hasActiveWorkout remains
true; wrap the selector in the same PopScope (or move the existing PopScope so
it encloses both branches) and reuse the same onPopInvokedWithResult behavior
that calls _handleSystemBack when didPop is false so hardware/gesture back
triggers the same cancel flow as the explicit close button; ensure you keep
canPop: false and reference provider.currentExerciseLogs,
_buildExerciseSelector, PopScope, and _handleSystemBack when making the change.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 49cc8ee9-f856-464a-9c9d-21c8f762b3c4
📒 Files selected for processing (6)
workout-logger/lib/screens/programs/program_detail_screen.dartworkout-logger/lib/screens/workout_flow_screen.dartworkout-logger/lib/services/workout_provider.dartworkout-logger/test/program_detail_screen_test.dartworkout-logger/test/test_utils/mock_storage_service.dartworkout-logger/test/workout_provider_test.dart
Comment on lines
+66
to
78
| ProgramDay? _resolvedProgramDay(WorkoutProvider provider) => | ||
| widget.programDay ?? provider.activeProgramDay; | ||
|
|
||
| ProgramWeek? _resolvedProgramWeek(WorkoutProvider provider) => | ||
| widget.programWeek ?? provider.activeProgramWeek; | ||
|
|
||
| ProgramExerciseSlot? _slotForIndex(int idx, {WorkoutProvider? provider}) { | ||
| final resolvedProvider = provider ?? context.read<WorkoutProvider>(); | ||
| final day = _resolvedProgramDay(resolvedProvider); | ||
| if (day == null) return null; | ||
| final slots = day.exercises; | ||
| return idx < slots.length ? slots[idx] : null; | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Defensive precedence for _resolvedProgramDay / _resolvedProgramWeek.
When provider.hasActiveWorkout is true, the resolved program context drives slot lookup, deload targets, rest timers, and superset detection across the whole screen (_slotForIndex, _supersetGroupStart, _supersetNeedsMoreSets, _buildProgramMetaBanner, _completeSet). Today the resolver prefers widget.programDay/Week, which is fine for every caller in this PR, but it’s easy to regress: any future entry point that pushes WorkoutFlowScreen with a stale programDay while another program workout is active will render the wrong context against the live session.
Consider giving the active workout precedence whenever it exists, falling back to the widget’s args only when there’s no active session:
♻️ Proposed precedence flip
- ProgramDay? _resolvedProgramDay(WorkoutProvider provider) =>
- widget.programDay ?? provider.activeProgramDay;
-
- ProgramWeek? _resolvedProgramWeek(WorkoutProvider provider) =>
- widget.programWeek ?? provider.activeProgramWeek;
+ ProgramDay? _resolvedProgramDay(WorkoutProvider provider) =>
+ provider.hasActiveWorkout
+ ? (provider.activeProgramDay ?? widget.programDay)
+ : (widget.programDay ?? provider.activeProgramDay);
+
+ ProgramWeek? _resolvedProgramWeek(WorkoutProvider provider) =>
+ provider.hasActiveWorkout
+ ? (provider.activeProgramWeek ?? widget.programWeek)
+ : (widget.programWeek ?? provider.activeProgramWeek);📝 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.
Suggested change
| ProgramDay? _resolvedProgramDay(WorkoutProvider provider) => | |
| widget.programDay ?? provider.activeProgramDay; | |
| ProgramWeek? _resolvedProgramWeek(WorkoutProvider provider) => | |
| widget.programWeek ?? provider.activeProgramWeek; | |
| ProgramExerciseSlot? _slotForIndex(int idx, {WorkoutProvider? provider}) { | |
| final resolvedProvider = provider ?? context.read<WorkoutProvider>(); | |
| final day = _resolvedProgramDay(resolvedProvider); | |
| if (day == null) return null; | |
| final slots = day.exercises; | |
| return idx < slots.length ? slots[idx] : null; | |
| } | |
| ProgramDay? _resolvedProgramDay(WorkoutProvider provider) => | |
| provider.hasActiveWorkout | |
| ? (provider.activeProgramDay ?? widget.programDay) | |
| : (widget.programDay ?? provider.activeProgramDay); | |
| ProgramWeek? _resolvedProgramWeek(WorkoutProvider provider) => | |
| provider.hasActiveWorkout | |
| ? (provider.activeProgramWeek ?? widget.programWeek) | |
| : (widget.programWeek ?? provider.activeProgramWeek); | |
| ProgramExerciseSlot? _slotForIndex(int idx, {WorkoutProvider? provider}) { | |
| final resolvedProvider = provider ?? context.read<WorkoutProvider>(); | |
| final day = _resolvedProgramDay(resolvedProvider); | |
| if (day == null) return null; | |
| final slots = day.exercises; | |
| return idx < slots.length ? slots[idx] : null; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workout-logger/lib/screens/workout_flow_screen.dart` around lines 66 - 78,
The current resolvers _resolvedProgramDay and _resolvedProgramWeek prefer
widget.programDay/Week over the provider’s active values, which can render stale
context when a live workout exists; change both functions to prefer
provider.activeProgramDay/activeProgramWeek whenever provider.hasActiveWorkout
(and that active value is non-null), falling back to widget.programDay/Week only
when there is no active session or the provider value is null; ensure
_slotForIndex (which calls _resolvedProgramDay and accepts an optional provider)
continues to pass through the resolved provider so lookups use the new
precedence.
Comment on lines
+188
to
+275
| Future<void> _restoreDraftIfAny() async { | ||
| String? rawDraft; | ||
| try { | ||
| rawDraft = await _storage.getSetting(_draftKey); | ||
| } catch (e) { | ||
| debugPrint('Failed to read active workout draft: $e'); | ||
| return; | ||
| } | ||
|
|
||
| if (rawDraft == null || rawDraft.isEmpty) { | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| final decoded = jsonDecode(rawDraft); | ||
| if (decoded is! Map) { | ||
| throw const FormatException('Draft payload must be a JSON object.'); | ||
| } | ||
| final draft = Map<String, dynamic>.from(decoded); | ||
|
|
||
| final schemaVersion = draft['schemaVersion']; | ||
| if (schemaVersion != _draftSchemaVersion) { | ||
| debugPrint( | ||
| 'Unsupported active workout draft schema: $schemaVersion. Clearing draft.', | ||
| ); | ||
| await _clearDraft(); | ||
| return; | ||
| } | ||
|
|
||
| final startTimeRaw = draft['startTime'] as String?; | ||
| final logsRaw = draft['currentExerciseLogs']; | ||
| if (startTimeRaw == null || logsRaw is! List) { | ||
| throw const FormatException('Draft is missing required fields.'); | ||
| } | ||
|
|
||
| final restoredLogs = logsRaw | ||
| .map( | ||
| (log) => | ||
| ExerciseLog.fromJson(Map<String, dynamic>.from(log as Map)), | ||
| ) | ||
| .toList(); | ||
|
|
||
| final programDayRaw = draft['programDay']; | ||
| ProgramDay? restoredProgramDay; | ||
| if (programDayRaw is Map) { | ||
| restoredProgramDay = ProgramDay.fromJson( | ||
| Map<String, dynamic>.from(programDayRaw), | ||
| ); | ||
| } | ||
|
|
||
| final programWeekRaw = draft['programWeek']; | ||
| ProgramWeek? restoredProgramWeek; | ||
| if (programWeekRaw is Map) { | ||
| restoredProgramWeek = ProgramWeek.fromJson( | ||
| Map<String, dynamic>.from(programWeekRaw), | ||
| ); | ||
| } | ||
|
|
||
| final routineId = draft['routineId'] as String?; | ||
| Routine? restoredRoutine; | ||
| if (routineId != null) { | ||
| try { | ||
| restoredRoutine = _routines.firstWhere((r) => r.id == routineId); | ||
| } catch (_) { | ||
| restoredRoutine = null; | ||
| } | ||
| } | ||
|
|
||
| final indexFromDraft = | ||
| (draft['currentExerciseIndex'] as num?)?.toInt() ?? 0; | ||
| final maxIndex = restoredLogs.isEmpty ? 0 : restoredLogs.length - 1; | ||
|
|
||
| _draftRestoreInProgress = true; | ||
| _activeSession = null; | ||
| _workoutStartTime = DateTime.parse(startTimeRaw); | ||
| _activeRoutine = restoredRoutine; | ||
| _activeProgramDay = restoredProgramDay; | ||
| _activeProgramWeek = restoredProgramWeek; | ||
| _currentExerciseLogs = restoredLogs; | ||
| _currentExerciseIndex = indexFromDraft.clamp(0, maxIndex).toInt(); | ||
| notifyListeners(); | ||
| } catch (e) { | ||
| debugPrint('Failed to restore active workout draft: $e'); | ||
| await _clearDraft(); | ||
| } finally { | ||
| _draftRestoreInProgress = false; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Defensive: clear the draft on stale-state shapes too.
_restoreDraftIfAny treats only schema-version mismatch and parse exceptions as “clear and bail.” A subtle gap: if a future schema regression yields a payload where currentExerciseIndex references an index past restoredLogs.length (e.g. a partial truncation), the clamp at line 267 silently lands the user on an arbitrary slot rather than surfacing the inconsistency. Consider validating that indexFromDraft is within [0, restoredLogs.length - 1] (when logs are non-empty) and either log+clear when out-of-range, or accept the clamp and add a debug log so it doesn’t fail silently. Not blocking — current behavior is graceful degradation — but a log line will pay for itself in diagnostics.
Also nit: _activeSession = null on line 261 is redundant (already null at init) and .toInt() on line 267 is a no-op since int.clamp(int, int) already returns int.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workout-logger/lib/services/workout_provider.dart` around lines 188 - 275, In
_restoreDraftIfAny validate the restored index against the restoredLogs length:
compute maxIndex = restoredLogs.isEmpty ? 0 : restoredLogs.length - 1 and if
restoredLogs.isNotEmpty and indexFromDraft is outside 0..maxIndex then
debugPrint a clear message and await _clearDraft() and return (or alternatively
log+clear), otherwise set _currentExerciseIndex = indexFromDraft.clamp(0,
maxIndex) (remove the redundant .toInt()); also remove the redundant assignment
_activeSession = null since it's already null. Ensure you reference
indexFromDraft, restoredLogs, maxIndex, _clearDraft, _currentExerciseIndex, and
_activeSession in the changes.
This was referenced Apr 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Tests