feat: Implement HistoryManager for managing workout session history - #19
Merged
Conversation
- Added HistoryManager class to handle loading, saving, updating, and deleting workout sessions. - Introduced methods for querying session history, including filtering by exercise and date range. feat: Create RoutineManager for managing workout routines - Added RoutineManager class to manage workout routines, including creating, updating, and deleting routines. - Implemented methods to load routines from storage and check exercise usage in routines. feat: Develop TargetManager for managing workout targets/goals - Introduced TargetManager class to handle target creation, updating, and progress tracking. - Implemented growth model updates and target completion predictions using a strategy pattern. refactor: Update MLService to implement IMLService interface - Refactored MLService to follow Dependency Inversion Principle, allowing for easier swapping of ML algorithms. - Enhanced methods for training growth models and generating set recommendations. refactor: Implement StorageService with IStorageService interface - Created StorageService class for local persistence using Hive, adhering to Dependency Inversion Principle. - Added methods for managing workout sessions, routines, targets, and muscle groups. feat: Introduce TargetCalculator strategy pattern for target value calculations - Implemented TargetCalculatorStrategy interface and various calculators for different target types. - Added factory for creating target calculators, allowing for easy extension of target types. refactor: Update WorkoutProvider to use individual managers - Refactored WorkoutProvider to utilize new managers for active workouts, history, routines, targets, and analytics. - Maintained backward compatibility while improving code structure and adherence to SOLID principles. test: Add mock services for ML and storage for testing - Created MockMLService and MockStorageService to facilitate testing of the application. - Implemented methods to simulate behavior of real services, allowing for isolated unit tests.
Contributor
WalkthroughAdds SOLID refactor: introduces Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
workout-logger/lib/services/ml_service.dart (1)
278-305: 🧹 Nitpick | 🔵 Trivial
predictTargetWithConfidencecreates an internal MLService instance.This static method instantiates
MLService()internally, which works but reduces testability—callers cannot inject a mock. Consider accepting anIMLServiceparameter or converting this to an instance method.♻️ Proposed refactor for testability
- static ({DateTime optimistic, DateTime expected, DateTime pessimistic})?- predictTargetWithConfidence({+ ({DateTime optimistic, DateTime expected, DateTime pessimistic})?+ predictTargetWithConfidenceInstance({ required double currentValue, required double targetValue, required GrowthModel growthModel, double sessionsPerWeek = 3.0, }) { - // Create instance to call the non-static method- final mlService = MLService();- final expected = mlService.predictTargetCompletion(+ final expected = predictTargetCompletion( currentValue: currentValue, targetValue: targetValue, growthModel: growthModel, sessionsPerWeek: sessionsPerWeek, ); // ... rest of method } ++ /// Static version for backward compatibility+ static ({DateTime optimistic, DateTime expected, DateTime pessimistic})?+ predictTargetWithConfidence({+ required double currentValue,+ required double targetValue,+ required GrowthModel growthModel,+ double sessionsPerWeek = 3.0,+ }) {+ return MLService().predictTargetWithConfidenceInstance(+ currentValue: currentValue,+ targetValue: targetValue,+ growthModel: growthModel,+ sessionsPerWeek: sessionsPerWeek,+ );+ }workout-logger/lib/services/workout_provider.dart (1)
617-656: 🧹 Nitpick | 🔵 TrivialDuplicated fallback logic may drift from strategy implementations.
The fallback code (lines 627-656) duplicates the logic in
RepsTargetCalculator,WeightTargetCalculator, andVolumeTargetCalculator. If the strategy implementations are updated, this fallback could become inconsistent.Consider removing the fallback and throwing an error for unknown types instead, or logging a warning and returning 0 for truly unknown types only.
♻️ Suggested refactor
double _calculateCurrentTargetValue(String exerciseId, String targetType) { // Try to use the strategy pattern first final calculator = TargetCalculatorFactory.getCalculator(targetType); if (calculator != null) { return calculator.calculate(exerciseId, _sessions); } - // Fallback for backward compatibility with unknown types- double bestValue = 0;-- for (var session in _sessions) {- for (var log in session.exercises) {- if (log.exerciseId == exerciseId && log.sets.isNotEmpty) {- double sessionValue = 0;- switch (targetType) {- case 'reps':- sessionValue = log.sets- .map((s) => s.reps)- .reduce((a, b) => a > b ? a : b)- .toDouble();- break;- case 'weight':- sessionValue = log.sets- .map((s) => s.weight)- .reduce((a, b) => a > b ? a : b);- break;- case 'volume':- sessionValue = log.totalVolume;- break;- }- if (sessionValue > bestValue) {- bestValue = sessionValue;- }- }- }- }- return bestValue;+ // Unknown target type - log warning and return 0+ debugPrint('Warning: Unknown target type "$targetType", returning 0');+ return 0; }
🤖 Fix all issues with AI agents
In `@SOLID_ANALYSIS_REPORT.md`:
- Around line 3-41: Remove the trailing space on line 3 in
SOLID_ANALYSIS_REPORT.md and fix heading spacing by ensuring each heading (e.g.,
"## ✅ SOLID Refactoring Complete" and the "####" headings such as "#### 1.
Dependency Inversion Principle (DIP)", "#### 2. Single Responsibility Principle
(SRP)", "#### 3. Open/Closed Principle (OCP)", "#### 4. Interface Segregation
Principle (ISP)", and "#### 5. Liskov Substitution Principle (LSP)") is
surrounded by a blank line above and below as the linter expects; update the
markdown file accordingly so there is an empty line before and after each of
those headings and remove any trailing whitespace.
In `@workout-logger/lib/main.dart`:
- Around line 44-60: The StorageService and MLService are instantiated inside
build(), causing new instances on every rebuild; move their creation out of
build() (e.g., instantiate them in main() or provide them via
Provider.create/lazy) and then supply those singletons into MultiProvider so the
providers use the same IStorageService and IMLService instances; update the
change notifier creation to still pass the same storageService and mlService
into WorkoutProvider so no multiple Hive initializations or duplicated ML
service instances occur.
In `@workout-logger/lib/services/interfaces/ml_service_interface.dart`:
- Around line 11-17: DataPoint is a value object but lacks equality semantics;
add an override of operator ==(Object other) and hashCode on the DataPoint class
(or alternatively implement Equatable) so two DataPoint instances with the same
x and y compare equal and can be used in sets/maps and assertions; update
DataPoint by comparing runtimeType and x/y in operator== and compute hashCode
from x and y to ensure consistent behavior.
In `@workout-logger/lib/services/interfaces/storage_service_interface.dart`:
- Around line 9-71: IStorageService is very broad and may violate the Interface
Segregation Principle; split it into focused repository interfaces (e.g.,
ISessionRepository, IRoutineRepository, ITargetRepository,
IMuscleGroupRepository, IExerciseRepository, ISettingsRepository,
IExportImportRepository, IStatsRepository) by moving the related methods (e.g.,
saveWorkoutSession/getAllWorkoutSessions/getWorkoutSession/deleteWorkoutSession/getSessionsForExercise/getSessionsInDateRange
-> ISessionRepository; saveRoutine/getAllRoutines/getRoutine/deleteRoutine ->
IRoutineRepository;
saveTarget/getAllTargets/getTarget/deleteTarget/getTargetsForExercise ->
ITargetRepository; updateMuscleGroupGrowthRate/getAllMuscleGroups/getMuscleGroup
-> IMuscleGroupRepository;
saveCustomExercise/getCustomExercises/deleteCustomExercise/getAllExercises/getExercise
-> IExerciseRepository; saveSetting/getSetting -> ISettingsRepository;
exportAllData/importData -> IExportImportRepository; getQuickStats ->
IStatsRepository), then have your concrete StorageService implement/compose
these smaller interfaces and update DI registrations to provide the specific
interfaces instead of the monolith IStorageService.
In `@workout-logger/lib/services/managers/active_workout_manager.dart`:
- Around line 162-196: finishWorkout currently calls
_storage.saveWorkoutSession(session) without explicit error handling which
leaves the workout state active if saving fails; wrap the save call in a
try/catch around _storage.saveWorkoutSession(session), log or surface the caught
error (so callers can see the failure), rethrow the exception to preserve
current behavior, and ensure _resetState() and onWorkoutSaved?.call(session) are
only executed after a successful save; additionally add a short doc comment to
finishWorkout noting that the active workout remains if saving fails so callers
must handle errors.
In `@workout-logger/lib/services/managers/analytics_manager.dart`:
- Around line 165-171: The _findExercise function currently uses firstWhere
inside a try/catch; update it to follow the project's consistent pattern (as in
HistoryManager.getSession) by using indexWhere to locate the exercise and return
exercises[index] when index != -1, otherwise return null; alternatively,
implement/use the shared utility/extension (e.g., a nullableFirstWhere
extension) used across the codebase and call that from _findExercise to remove
the try/catch and keep behavior consistent.
- Around line 88-113: getRecommendations currently picks the first matching
ExerciseLog by iterating sessions which only works if sessions are already
newest-first; modify getRecommendations to enforce ordering by creating a copy
of sessions and sorting it newest-first (e.g., sort by WorkoutSession.date
descending) before searching for the last log, or alternatively document the
newest-first requirement in the method doc comment; ensure you still call
_mlService.recommendSets with lastSession: lastLog.sets and pass the correct
_growthModels[exerciseId].
In `@workout-logger/lib/services/managers/exercise_manager.dart`:
- Around line 46-53: The getExercise(String id) method currently falls back to
ExerciseDatabase.getById(id), creating an implicit, non-injected secondary data
source; remove that hidden dependency by either (A) eliminating the fallback in
Exercise? getExercise(String id) and returning null/throwing when not found so
callers rely on the manager's loaded state, or (B) inject an ExerciseDatabase
dependency into ExerciseManager (e.g., via constructor) and use that injected
instance instead of calling ExerciseDatabase.getById directly; alternatively
ensure loadExercises() always loads built-in exercises into _allExercises before
callers invoke getExercise — update tests accordingly to reflect the chosen
approach.
- Around line 154-157: getExercisesByCategory currently compares category
strings directly which is case-sensitive while addCustomExercise normalizes
categories to lowercase; update getExercisesByCategory to perform a
case-insensitive match by normalizing the input category and each
Exercise.category (refer to getExercisesByCategory, _allExercises, and the
Exercise.category property) before comparing so built-in and custom exercises
with different casing (e.g., "Compound" vs "compound") are returned
consistently.
In `@workout-logger/lib/services/managers/history_manager.dart`:
- Around line 134-144: getLastSessionForExercise currently assumes _sessions is
sorted newest-first but loadSessions doesn't guarantee order; ensure _sessions
is sorted by session date (newest first) before returning or iterating. Update
loadSessions to sort _sessions (or add a local sort at the start of
getLastSessionForExercise) using the session date field so
getLastSessionForExercise, which is in the HistoryManager class, reliably
returns the most recent ExerciseLog; you can mirror the sorting logic used in
updateSession to keep behavior consistent.
- Around line 66-73: The getSessionsInDateRange method in HistoryManager
currently uses exclusive bounds (session.date.isAfter(start) &&
session.date.isBefore(end)), which omits sessions exactly equal to start or end;
update the predicate in getSessionsInDateRange to use inclusive checks (e.g.,
!session.date.isBefore(start) && !session.date.isAfter(end) or equivalent
comparisons) so sessions on the boundary dates are included.
- Around line 48-54: Replace the try/catch in getSession with a non-exception
control flow: use IterableExtension.firstWhereOrNull from package:collection (or
firstWhere with an orElse) to return a nullable WorkoutSession. Concretely,
change the body of getSession to return _sessions.firstWhereOrNull((s) => s.id
== sessionId) and add import 'package:collection/collection.dart';
alternatively, use _sessions.firstWhere((s) => s.id == sessionId, orElse: () =>
null) if you prefer not to add the package.
- Around line 103-125: In updateSession, previousSession is set to
updatedSession when firstWhere misses, which hides the "not found" case and
causes _storage.saveWorkoutSession(updatedSession) to run; change the logic to
check existence first (use index = _sessions.indexWhere((s) => s.id ==
updatedSession.id)), if index == -1 either throw a descriptive error or return
early without calling _storage.saveWorkoutSession, otherwise set previousSession
= _sessions[index], compute previousExerciseIds from that, then continue to save
and replace _sessions[index] with updatedSession.
- Around line 40-45: addSession currently only mutates in-memory _sessions; to
match deleteSession and updateSession it must persist the new session by calling
_storage.saveWorkoutSession(session). Change addSession (in HistoryManager) to
be async, call await _storage.saveWorkoutSession(session) after invoking
onSessionsChanged and before notifyListeners, and update any callers to await
the new async signature; alternatively, if persistence should remain
caller-responsibility, add a doc comment to addSession explicitly stating it
does not persist and leave callers responsible.
In `@workout-logger/lib/services/managers/routine_manager.dart`:
- Around line 59-67: The updateRoutine method currently saves to _storage but
only updates _routines when an existing item is found, leaving in-memory state
out of sync; modify updateRoutine so that after await
_storage.saveRoutine(routine) you check index = _routines.indexWhere((r) => r.id
== routine.id) and if index != -1 replace _routines[index] = routine, else add
the routine to _routines (e.g., _routines.add(routine) or insert at the desired
position) before calling notifyListeners(); keep the save call and
notifyListeners() behavior intact and reference updateRoutine,
_storage.saveRoutine, and _routines when making the change.
- Around line 38-44: The getRoutine method currently uses try/catch to handle a
missing element from _routines.firstWhere; replace that pattern by supplying the
orElse parameter to firstWhere (e.g., _routines.firstWhere((r) => r.id == id,
orElse: () => null)) or use collection's firstWhereOrNull to return null
directly; update the getRoutine implementation to call _routines.firstWhere(...,
orElse: () => null) or _routines.firstWhereOrNull((r) => r.id == id) instead of
catching exceptions.
In `@workout-logger/lib/services/managers/target_manager.dart`:
- Around line 66-106: createTarget currently calls
TargetCalculatorFactory.calculateCurrentValue which throws ArgumentError for
unsupported target types; add an upfront validation before that call to check
the type and fail fast with a clearer message (e.g., detect supported types via
an existing helper on TargetCalculatorFactory or add a isSupported/isValidType
method) and throw a descriptive ArgumentError from createTarget (including the
invalid type and allowed values) instead of letting the error bubble
mid-operation; ensure the validation occurs before calling calculateCurrentValue
and that the thrown error is the same/consistent type the rest of the code
expects.
In `@workout-logger/lib/services/storage_service.dart`:
- Around line 99-110: The date-range filter in getSessionsInDateRange currently
excludes sessions on the exact start/end because it uses
session.date.isAfter(start) and session.date.isBefore(end); change the predicate
to include boundary dates by checking for equality or using the inverse of
isBefore/isAfter (e.g., session.date.isAtSameMomentAs(start) ||
session.date.isAfter(start) and session.date.isAtSameMomentAs(end) ||
session.date.isBefore(end), or equivalently !session.date.isBefore(start) &&
!session.date.isAfter(end)) so sessions occurring exactly on start or end are
returned; update the where(...) condition in getSessionsInDateRange (which works
on items from getAllWorkoutSessions) accordingly.
In `@workout-logger/lib/services/strategies/target_calculator.dart`:
- Around line 89-112: Add a static reset method on TargetCalculatorFactory that
restores the internal static mutable registry _strategies to its original
defaults (re-populate 'reps', 'weight', 'volume' with RepsTargetCalculator(),
WeightTargetCalculator(), VolumeTargetCalculator()) so tests can call
TargetCalculatorFactory.reset() to restore isolation; update registerCalculator
and getCalculator usage remains the same, and add the suggested import
(package:flutter/foundation.dart) if required by your test utilities.
In `@workout-logger/test/test_utils/mock_storage_service.dart`:
- Around line 51-56: The mock's getAllExercises currently returns only
_customExercises which diverges from the real StorageService.getAllExercises
(which merges built-in exercises from ExerciseDatabase.getAll() with custom
ones); update mock_storage_service.dart so getAllExercises calls or merges
results from ExerciseDatabase.getAll() with _customExercises (e.g., fetch
builtins then return a combined List) to mirror production behavior, or if
intentionally simplified, add a clear comment in the mock near getAllExercises
explaining it only returns custom exercises and may not reflect real behavior.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Comment on lines
+9
to
+71
| /// Abstract interface for storage operations | ||
| /// | ||
| /// Implements Interface Segregation Principle by being focused on storage concerns only. | ||
| /// Clients depend on this abstraction rather than concrete StorageService. | ||
| abstract class IStorageService { | ||
| /// Initialize the storage backend | ||
| Future<void> init(); | ||
| // ==================== WORKOUT SESSIONS ==================== | ||
| Future<void> saveWorkoutSession(WorkoutSession session); | ||
| Future<List<WorkoutSession>> getAllWorkoutSessions(); | ||
| Future<WorkoutSession?> getWorkoutSession(String id); | ||
| Future<void> deleteWorkoutSession(String id); | ||
| Future<List<WorkoutSession>> getSessionsForExercise(String exerciseId); | ||
| Future<List<WorkoutSession>> getSessionsInDateRange( | ||
| DateTime start, | ||
| DateTime end, | ||
| ); | ||
| // ==================== ROUTINES ==================== | ||
| Future<void> saveRoutine(Routine routine); | ||
| Future<List<Routine>> getAllRoutines(); | ||
| Future<Routine?> getRoutine(String id); | ||
| Future<void> deleteRoutine(String id); | ||
| // ==================== TARGETS ==================== | ||
| Future<void> saveTarget(Target target); | ||
| Future<List<Target>> getAllTargets(); | ||
| Future<Target?> getTarget(String id); | ||
| Future<void> deleteTarget(String id); | ||
| Future<List<Target>> getTargetsForExercise(String exerciseId); | ||
| // ==================== MUSCLE GROUPS ==================== | ||
| Future<void> updateMuscleGroupGrowthRate(String muscleGroupId, double rate); | ||
| Future<List<MuscleGroup>> getAllMuscleGroups(); | ||
| Future<MuscleGroup?> getMuscleGroup(String id); | ||
| // ==================== CUSTOM EXERCISES ==================== | ||
| Future<void> saveCustomExercise(Exercise exercise); | ||
| Future<List<Exercise>> getCustomExercises(); | ||
| Future<void> deleteCustomExercise(String id); | ||
| Future<List<Exercise>> getAllExercises(); | ||
| Future<Exercise?> getExercise(String id); | ||
| // ==================== SETTINGS ==================== | ||
| Future<void> saveSetting(String key, String value); | ||
| Future<String?> getSetting(String key); | ||
| // ==================== EXPORT / IMPORT ==================== | ||
| Future<String> exportAllData(); | ||
| Future<void> importData(String jsonData); | ||
| // ==================== STATS ==================== | ||
| Future<Map<String, dynamic>> getQuickStats(); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Interface enables DI but consider future ISP refinement.
The IStorageService interface successfully decouples consumers from the Hive implementation, addressing the DIP violation identified in learnings. The interface is well-organized with clear categorical sections.
However, the learnings also noted ISP violations where components depend on functionality they don't use. This interface is still quite broad (29+ methods). As a future improvement, consider splitting into smaller, focused interfaces (e.g., ISessionRepository, IRoutineRepository) that can be composed as needed.
For now, this is a solid first step that enables the immediate DI goals.
Would you like me to open an issue to track the potential ISP refinement into smaller repository interfaces?
🤖 Prompt for AI Agents
In `@workout-logger/lib/services/interfaces/storage_service_interface.dart` around
lines 9 - 71, IStorageService is very broad and may violate the Interface
Segregation Principle; split it into focused repository interfaces (e.g.,
ISessionRepository, IRoutineRepository, ITargetRepository,
IMuscleGroupRepository, IExerciseRepository, ISettingsRepository,
IExportImportRepository, IStatsRepository) by moving the related methods (e.g.,
saveWorkoutSession/getAllWorkoutSessions/getWorkoutSession/deleteWorkoutSession/getSessionsForExercise/getSessionsInDateRange
-> ISessionRepository; saveRoutine/getAllRoutines/getRoutine/deleteRoutine ->
IRoutineRepository;
saveTarget/getAllTargets/getTarget/deleteTarget/getTargetsForExercise ->
ITargetRepository; updateMuscleGroupGrowthRate/getAllMuscleGroups/getMuscleGroup
-> IMuscleGroupRepository;
saveCustomExercise/getCustomExercises/deleteCustomExercise/getAllExercises/getExercise
-> IExerciseRepository; saveSetting/getSetting -> ISettingsRepository;
exportAllData/importData -> IExportImportRepository; getQuickStats ->
IStatsRepository), then have your concrete StorageService implement/compose
these smaller interfaces and update DI registrations to provide the specific
interfaces instead of the monolith IStorageService.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
… workout flow screens
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@workout-logger/lib/screens/routines_screen.dart`:
- Line 272: _pickerSearchQuery is declared as class-level state but is only used
inside _showExercisePicker's StatefulBuilder; move it to a local variable inside
_showExercisePicker (e.g., declare String pickerSearchQuery = '' before calling
showModal or inside the method) and update the StatefulBuilder to capture and
mutate that local via setState from the builder rather than referencing the
class field; remove the class-level _pickerSearchQuery declaration to avoid
cross-rebuild leakage and ensure the picker’s search state is scoped to the
modal.
In `@workout-logger/lib/screens/workout_flow_screen.dart`:
- Around line 1117-1121: An inline TextEditingController is being created in the
bottom sheet (controller: TextEditingController(...)) and is never disposed,
causing a memory leak; replace the inline controller with a dedicated
StatefulWidget (e.g., _NumberPickerContent) that creates the controller in
initState and disposes it in dispose, accept initialValue, decimals and
onChanged props, and use that widget as the bottom sheet content (move the
TextField's controller to the stateful widget and forward onSubmitted/onPressed
events to Navigator.pop and onChanged).
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@workout-logger/lib/screens/routines_screen.dart`:
- Around line 461-472: The batch "Add" uses tempSelectedIds (a Set<String>) so
selection order is lost; change tempSelectedIds to a List<String> (e.g.,
List<String> tempSelectedIds) and update the toggle logic that mutates it to
maintain order by removing an id if present or appending it if not (keep the
toggle behavior but operate on the List), then when confirming use ordered
addition to _selectedExerciseIds (e.g., addAll/insertAll from the List) so
exercises are appended in selection order; update any UI/state references and
types for tempSelectedIds and the toggle handler to match List<String>.
In `@workout-logger/lib/services/managers/active_workout_manager.dart`:
- Around line 61-80: startWorkout currently allows starting a workout with zero
exercises because it falls back to an empty list (ids = routine?.exerciseIds ??
exerciseIds ?? []), which can create an empty _currentExerciseLogs; update
startWorkout to validate that ids is not empty (or explicitly document the
empty-workout behavior) by checking the resolved ids list after it is computed
and throwing a clear StateError (or returning a boolean) if no exercises are
provided, referencing the startWorkout method and private fields
_currentExerciseLogs, _activeRoutine, and hasActiveWorkout when adding the
validation and error path.
In `@workout-logger/lib/services/managers/analytics_manager.dart`:
- Around line 41-55: trainAllGrowthModels currently iterates exerciseIds and
awaits updateGrowthModel sequentially, which is slow for many exercises; change
it to launch all updateGrowthModel calls concurrently using a collection of
futures and await Future.wait(...) (or use a bounded concurrency pool if you
need to limit parallelism) so that updateGrowthModel(exerciseId, sessions) runs
in parallel for all exerciseIds instead of one-by-one.
In `@workout-logger/lib/services/strategies/target_calculator.dart`:
- Around line 20-83: Extract the common nested loop into a single helper
function named _allSetsForExercise(String exerciseId, List<WorkoutSession>
sessions) that yields/returns all WorkoutSet instances for the given exercise
across sessions, then rewrite RepsTargetCalculator.calculate,
WeightTargetCalculator.calculate and VolumeTargetCalculator.calculate to use
that helper (checking for empty iterable before calling reduce and converting
types where needed), removing the duplicated for-loops in each calculator and
preserving existing behavior (max reps -> double, max weight, and max
totalVolume derived from sets/logs as appropriate).
In `@workout-logger/test/test_utils/mock_storage_service.dart`:
- Around line 112-123: The mock getSessionsInDateRange currently filters with
exclusive bounds (session.date.isAfter(start) && session.date.isBefore(end));
change it to use inclusive bounds to match production by checking
!session.date.isBefore(start) && !session.date.isAfter(end) when filtering
_sessions (i.e., update the predicate in getSessionsInDateRange to use those
comparisons so boundary dates are included).
- Around line 208-215: The mock getExercise implementation only searches
_customExercises but should mirror production StorageService.getExercise by
first querying ExerciseDatabase.getById(id) for built-in exercises and returning
that if non-null, then falling back to searching _customExercises (using the
existing firstWhere logic), returning null only if neither source finds the
exercise; update the getExercise method in mock_storage_service.dart to call
ExerciseDatabase.getById(id) first and return its result when present.
Comment on lines
+461
to
+472
| TextButton.icon( | ||
| onPressed: () { | ||
| setState(() { | ||
| _selectedExerciseIds.addAll( | ||
| tempSelectedIds, | ||
| ); | ||
| }); | ||
| Navigator.pop(context); | ||
| }, | ||
| icon: const Icon(Icons.check), | ||
| label: Text('Add ${tempSelectedIds.length}'), | ||
| ), |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Selection order may not be preserved when batch-adding exercises.
tempSelectedIds is a Set<String>, so when the user taps "Add", the exercises are added in hash iteration order rather than the order they were selected. If selection order matters for routine exercise ordering, consider using a List<String> instead.
♻️ Preserve selection order
- final Set<String> tempSelectedIds = {};+ final List<String> tempSelectedIds = [];Then update the toggle logic:
if (val == true) {
- tempSelectedIds.add(exercise.id);+ if (!tempSelectedIds.contains(exercise.id)) {+ tempSelectedIds.add(exercise.id);+ }
} else {
tempSelectedIds.remove(exercise.id);
}🤖 Prompt for AI Agents
In `@workout-logger/lib/screens/routines_screen.dart` around lines 461 - 472, The
batch "Add" uses tempSelectedIds (a Set<String>) so selection order is lost;
change tempSelectedIds to a List<String> (e.g., List<String> tempSelectedIds)
and update the toggle logic that mutates it to maintain order by removing an id
if present or appending it if not (keep the toggle behavior but operate on the
List), then when confirming use ordered addition to _selectedExerciseIds (e.g.,
addAll/insertAll from the List) so exercises are appended in selection order;
update any UI/state references and types for tempSelectedIds and the toggle
handler to match List<String>.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…ock storage service behavior
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@workout-logger/lib/screens/routines_screen.dart`:
- Around line 512-586: The tempSelectedIds list can receive duplicate IDs due to
rapid toggles from the Checkbox and ListTile; inside the Checkbox onChanged and
the ListTile onTap handlers (the closures that call setModalState), add guards
that check tempSelectedIds.contains(exercise.id) before calling
tempSelectedIds.add(exercise.id) and similarly ensure removal uses
tempSelectedIds.remove(exercise.id) only if present—this prevents adding
duplicates while keeping tempSelectedIds as a List to preserve selection order;
update both handlers (the Checkbox onChanged and the ListTile onTap blocks that
reference isSelected and tempSelectedIds) to perform these contains checks.
In `@workout-logger/lib/services/managers/active_workout_manager.dart`:
- Around line 210-215: The call to onWorkoutSaved?.call(session) can throw and
prevent cleanup; wrap the callback invocation in a try block and move the state
cleanup calls (_resetState() and notifyListeners()) into a finally block so the
active workout is cleared regardless of exceptions from onWorkoutSaved; keep the
session passed to the callback but ensure _resetState and notifyListeners always
run even if onWorkoutSaved throws.
In `@workout-logger/lib/services/managers/analytics_manager.dart`:
- Around line 79-88: updateGrowthModelsForExercises currently awaits
updateGrowthModel sequentially in a for-loop; change it to run updates in
parallel like trainAllGrowthModels by mapping exerciseIds to Futures (calling
updateGrowthModel for each id), await Future.wait on that list, then call
notifyListeners() once after all complete to preserve behavior and improve
performance; reference updateGrowthModelsForExercises, updateGrowthModel and
trainAllGrowthModels when making the change.
- Around line 40-57: trainAllGrowthModels populates/updates _growthModels by
calling updateGrowthModel for each exercise but never calls notifyListeners(),
causing listeners to miss the bulk update; add a notifyListeners() call at the
end of trainAllGrowthModels (after the Future.wait completes and _growthModels
has been updated) to mirror updateGrowthModelsForExercises behavior so UI
subscribers are informed of the changes.
- Around line 146-171: getWeeklyVolumeByMuscle uses DateTime.now() directly
which hinders testability; add an optional DateTime? now parameter (defaulting
to DateTime.now()) to getWeeklyVolumeByMuscle and use that parameter to compute
weekAgo instead of calling DateTime.now() so callers/tests can inject a fixed
timestamp; update any call sites of getWeeklyVolumeByMuscle to pass the current
time where needed (or omit to use the default) and adjust unit tests to supply a
deterministic DateTime when asserting week-based behavior.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…nup in workout manager
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
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/routines_screen.dart (1)
15-31: 🧹 Nitpick | 🔵 TrivialConsider reading narrower managers instead of the full
WorkoutProvider.This screen only needs routine data (and exercise names), so if the new Routine/Exercise managers are available via DI, prefer those to reduce coupling and rebuild scope.
Based on learnings, future changes should ensure components depend only on the specific interfaces or providers they actually need.
🤖 Fix all issues with AI agents
In `@workout-logger/lib/services/managers/active_workout_manager.dart`:
- Around line 95-143: The three methods have inconsistent error handling: addSet
throws when there is no active workout while removeLastSet and
updateCurrentExerciseNotes silently return; make the contract consistent by
having removeLastSet and updateCurrentExerciseNotes also throw the same
StateError when !hasActiveWorkout (use the same message as in addSet), so
callers get a uniform exception behavior; update removeLastSet and
updateCurrentExerciseNotes to throw when hasActiveWorkout is false and keep
existing logic that checks _currentExerciseIndex < _currentExerciseLogs.length
and notifies listeners unchanged.
- Around line 60-93: The startWorkout method currently uses ids directly so
duplicate exercise IDs produce multiple ExerciseLog entries; either deduplicate
ids (preserving order) before creating _currentExerciseLogs or explicitly
document that duplicates are allowed (e.g., for circuits). To dedupe, update
startWorkout (referenced symbols: startWorkout, ids, _currentExerciseLogs,
ExerciseLog, _currentExerciseIndex) to filter ids into an order-preserving
unique list (track a seen Set while iterating ids) and then create one
ExerciseLog per unique id; if duplicates are intentional, add a comment and
tests asserting duplicate behavior instead of changing logic.
In `@workout-logger/lib/services/managers/analytics_manager.dart`:
- Around line 43-56: The current trainAllGrowthModels uses Future.wait over all
exercise IDs which can spike resources; change trainAllGrowthModels to process
exerciseIds in bounded-concurrency batches (e.g., split exerciseIds into chunks
or use a simple semaphore/worker pool) and await each batch before starting the
next, calling updateGrowthModel(exerciseId, sessions) within each worker; apply
the same batching/limited-concurrency pattern to updateGrowthModelsForExercises
so both methods cap the number of simultaneous futures and avoid CPU/memory/UI
jank.
- Around line 164-166: The loop that filters sessions for the past week
currently only excludes sessions before weekAgo and can include future-dated
sessions; inside the same for-loop that iterates over sessions (the code
referencing sessions, weekAgo, and now in analytics_manager.dart) add a check to
also skip sessions with session.date after now (e.g., if
(session.date.isAfter(now)) continue;) so only sessions with weekAgo <=
session.date <= now are counted for weekly volume.
- Around line 62-79: updateGrowthModel mutates the private map _growthModels but
always invokes onGrowthModelUpdated, so callers doing bulk updates can't
suppress per-item notifications; add an optional boolean parameter (e.g. notify
= true) to updateGrowthModel signature and wrap the onGrowthModelUpdated call
(and any external notification/dispatch) behind if (notify) so callers can pass
notify: false during bulk operations, and ensure the branch that removes stale
models also respects notify; update bulk callers to pass notify: false as
needed.
Comment on lines
+60
to
+93
| /// Start a new workout with a routine or list of exercises | ||
| /// | ||
| /// Throws [StateError] if a workout is already in progress or if no | ||
| /// exercises are provided. | ||
| void startWorkout({Routine? routine, List<String>? exerciseIds}) { | ||
| if (hasActiveWorkout) { | ||
| throw StateError( | ||
| 'A workout is already in progress. Cancel or finish it first.', | ||
| ); | ||
| } | ||
| // Resolve exercise IDs from routine or provided list | ||
| final ids = routine?.exerciseIds ?? exerciseIds ?? []; | ||
| // Validate that at least one exercise is provided | ||
| if (ids.isEmpty) { | ||
| throw StateError( | ||
| 'Cannot start a workout with zero exercises. ' | ||
| 'Provide a routine with exercises or a non-empty exerciseIds list.', | ||
| ); | ||
| } | ||
| _workoutStartTime = DateTime.now(); | ||
| _activeRoutine = routine; | ||
| _currentExerciseIndex = 0; | ||
| _currentExerciseLogs = []; | ||
| // Initialize exercise logs based on routine or provided exercise IDs | ||
| for (var id in ids) { | ||
| _currentExerciseLogs.add(ExerciseLog(exerciseId: id, sets: [])); | ||
| } | ||
| notifyListeners(); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Verify duplicate exercise IDs are filtered before log creation.ids is used as-is, so duplicates become multiple ExerciseLog entries. If “prevent duplicate exercise selections” is a requirement at this layer, dedupe here (or document that duplicates are allowed for circuits).
🧩 Optional order-preserving dedupe
- final ids = routine?.exerciseIds ?? exerciseIds ?? [];+ final ids = routine?.exerciseIds ?? exerciseIds ?? [];+ final dedupedIds = <String>[];+ final seen = <String>{};+ for (final id in ids) {+ if (seen.add(id)) dedupedIds.add(id);+ }- if (ids.isEmpty) {+ if (dedupedIds.isEmpty) {
throw StateError(
'Cannot start a workout with zero exercises. '
'Provide a routine with exercises or a non-empty exerciseIds list.',
);
}
// Initialize exercise logs based on routine or provided exercise IDs
- for (var id in ids) {+ for (final id in dedupedIds) {
_currentExerciseLogs.add(ExerciseLog(exerciseId: id, sets: []));
}🤖 Prompt for AI Agents
In `@workout-logger/lib/services/managers/active_workout_manager.dart` around
lines 60 - 93, The startWorkout method currently uses ids directly so duplicate
exercise IDs produce multiple ExerciseLog entries; either deduplicate ids
(preserving order) before creating _currentExerciseLogs or explicitly document
that duplicates are allowed (e.g., for circuits). To dedupe, update startWorkout
(referenced symbols: startWorkout, ids, _currentExerciseLogs, ExerciseLog,
_currentExerciseIndex) to filter ids into an order-preserving unique list (track
a seen Set while iterating ids) and then create one ExerciseLog per unique id;
if duplicates are intentional, add a comment and tests asserting duplicate
behavior instead of changing logic.
Comment on lines
+95
to
+143
| /// Add a set to current exercise | ||
| void addSet(WorkoutSet set) { | ||
| if (!hasActiveWorkout) { | ||
| throw StateError('No active workout. Start a workout first.'); | ||
| } | ||
| if (_currentExerciseIndex < _currentExerciseLogs.length) { | ||
| final currentLog = _currentExerciseLogs[_currentExerciseIndex]; | ||
| _currentExerciseLogs[_currentExerciseIndex] = ExerciseLog( | ||
| exerciseId: currentLog.exerciseId, | ||
| sets: [...currentLog.sets, set], | ||
| notes: currentLog.notes, | ||
| ); | ||
| notifyListeners(); | ||
| } | ||
| } | ||
| /// Remove last set from current exercise | ||
| void removeLastSet() { | ||
| if (!hasActiveWorkout) return; | ||
| if (_currentExerciseIndex < _currentExerciseLogs.length) { | ||
| final currentLog = _currentExerciseLogs[_currentExerciseIndex]; | ||
| if (currentLog.sets.isNotEmpty) { | ||
| final newSets = List<WorkoutSet>.from(currentLog.sets)..removeLast(); | ||
| _currentExerciseLogs[_currentExerciseIndex] = ExerciseLog( | ||
| exerciseId: currentLog.exerciseId, | ||
| sets: newSets, | ||
| notes: currentLog.notes, | ||
| ); | ||
| notifyListeners(); | ||
| } | ||
| } | ||
| } | ||
| /// Update notes for current exercise | ||
| void updateCurrentExerciseNotes(String? notes) { | ||
| if (!hasActiveWorkout) return; | ||
| if (_currentExerciseIndex < _currentExerciseLogs.length) { | ||
| final currentLog = _currentExerciseLogs[_currentExerciseIndex]; | ||
| _currentExerciseLogs[_currentExerciseIndex] = ExerciseLog( | ||
| exerciseId: currentLog.exerciseId, | ||
| sets: currentLog.sets, | ||
| notes: notes, | ||
| ); | ||
| notifyListeners(); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Align error handling across add/remove/update operations.addSet throws on missing active workout, while removeLastSet / updateCurrentExerciseNotes silently return. That inconsistency can hide bugs in callers. Consider a unified contract (all throw, or all return a bool).
🔁 Example: make all throw for consistency
void removeLastSet() {
- if (!hasActiveWorkout) return;+ if (!hasActiveWorkout) {+ throw StateError('No active workout. Start a workout first.');+ }
if (_currentExerciseIndex < _currentExerciseLogs.length) {
final currentLog = _currentExerciseLogs[_currentExerciseIndex];
if (currentLog.sets.isNotEmpty) {
final newSets = List<WorkoutSet>.from(currentLog.sets)..removeLast();
_currentExerciseLogs[_currentExerciseIndex] = ExerciseLog(
exerciseId: currentLog.exerciseId,
sets: newSets,
notes: currentLog.notes,
);
notifyListeners();
}
}
}
void updateCurrentExerciseNotes(String? notes) {
- if (!hasActiveWorkout) return;+ if (!hasActiveWorkout) {+ throw StateError('No active workout. Start a workout first.');+ }
if (_currentExerciseIndex < _currentExerciseLogs.length) {
final currentLog = _currentExerciseLogs[_currentExerciseIndex];
_currentExerciseLogs[_currentExerciseIndex] = ExerciseLog(
exerciseId: currentLog.exerciseId,
sets: currentLog.sets,
notes: notes,
);
notifyListeners();
}
}🤖 Prompt for AI Agents
In `@workout-logger/lib/services/managers/active_workout_manager.dart` around
lines 95 - 143, The three methods have inconsistent error handling: addSet
throws when there is no active workout while removeLastSet and
updateCurrentExerciseNotes silently return; make the contract consistent by
having removeLastSet and updateCurrentExerciseNotes also throw the same
StateError when !hasActiveWorkout (use the same message as in addSet), so
callers get a uniform exception behavior; update removeLastSet and
updateCurrentExerciseNotes to throw when hasActiveWorkout is false and keep
existing logic that checks _currentExerciseIndex < _currentExerciseLogs.length
and notifies listeners unchanged.
Comment on lines
+43
to
+56
| Future<void> trainAllGrowthModels(List<WorkoutSession> sessions) async { | ||
| final exerciseIds = <String>{}; | ||
| // Get all unique exercise IDs from sessions | ||
| for (var session in sessions) { | ||
| for (var log in session.exercises) { | ||
| exerciseIds.add(log.exerciseId); | ||
| } | ||
| } | ||
| // Train models concurrently for better performance | ||
| await Future.wait( | ||
| exerciseIds.map((exerciseId) => updateGrowthModel(exerciseId, sessions)), | ||
| ); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Bound concurrency to avoid a burst of futures.Future.wait on a large exercise set can spike CPU/memory and jank UI. Consider batching with a small max concurrency; apply the same pattern to updateGrowthModelsForExercises.
♻️ Example batching (bounded concurrency)
Future<void> trainAllGrowthModels(List<WorkoutSession> sessions) async {
final exerciseIds = <String>{};
@@
- // Train models concurrently for better performance- await Future.wait(- exerciseIds.map((exerciseId) => updateGrowthModel(exerciseId, sessions)),- );+ // Train models with bounded concurrency to avoid spikes+ const maxConcurrent = 6;+ final ids = exerciseIds.toList();+ for (var i = 0; i < ids.length; i += maxConcurrent) {+ final end = (i + maxConcurrent < ids.length) ? i + maxConcurrent : ids.length;+ final batch = ids.sublist(i, end);+ await Future.wait(+ batch.map((exerciseId) => updateGrowthModel(exerciseId, sessions)),+ );+ }
// Notify listeners after bulk update completes
notifyListeners();
}🤖 Prompt for AI Agents
In `@workout-logger/lib/services/managers/analytics_manager.dart` around lines 43
- 56, The current trainAllGrowthModels uses Future.wait over all exercise IDs
which can spike resources; change trainAllGrowthModels to process exerciseIds in
bounded-concurrency batches (e.g., split exerciseIds into chunks or use a simple
semaphore/worker pool) and await each batch before starting the next, calling
updateGrowthModel(exerciseId, sessions) within each worker; apply the same
batching/limited-concurrency pattern to updateGrowthModelsForExercises so both
methods cap the number of simultaneous futures and avoid CPU/memory/UI jank.
Comment on lines
+62
to
+79
| /// Update growth model for a specific exercise | ||
| Future<void> updateGrowthModel( | ||
| String exerciseId, | ||
| List<WorkoutSession> sessions, | ||
| ) async { | ||
| final dataPoints = _mlService.extractExerciseDataPoints( | ||
| exerciseId, | ||
| sessions, | ||
| ); | ||
| if (dataPoints.length >= 2) { | ||
| final model = _mlService.trainGrowthModel(dataPoints); | ||
| _growthModels[exerciseId] = model; | ||
| onGrowthModelUpdated?.call(exerciseId, model); | ||
| } else { | ||
| // Remove stale model if not enough data | ||
| _growthModels.remove(exerciseId); | ||
| } |
Contributor
There was a problem hiding this comment.
State changes in updateGrowthModel are not notifying listeners.
This method is public and mutates _growthModels; callers that invoke it directly won’t see updates unless they remember to notify externally. Either make it private or add an optional notify flag and disable it in bulk calls.
🔔 Suggested fix with optional notification
Future<void> updateGrowthModel(
String exerciseId,
List<WorkoutSession> sessions,
+ {bool notify = true},
) async {
@@
} else {
// Remove stale model if not enough data
_growthModels.remove(exerciseId);
}
+ if (notify) {+ notifyListeners();+ }
}🧩 Update bulk callers to suppress per-item notifications
- await Future.wait(- exerciseIds.map((exerciseId) => updateGrowthModel(exerciseId, sessions)),- );+ await Future.wait(+ exerciseIds.map((exerciseId) =>+ updateGrowthModel(exerciseId, sessions, notify: false)),+ );
@@
- await Future.wait(- exerciseIds.map((exerciseId) => updateGrowthModel(exerciseId, sessions)),- );+ await Future.wait(+ exerciseIds.map((exerciseId) =>+ updateGrowthModel(exerciseId, sessions, notify: false)),+ );🤖 Prompt for AI Agents
In `@workout-logger/lib/services/managers/analytics_manager.dart` around lines 62
- 79, updateGrowthModel mutates the private map _growthModels but always invokes
onGrowthModelUpdated, so callers doing bulk updates can't suppress per-item
notifications; add an optional boolean parameter (e.g. notify = true) to
updateGrowthModel signature and wrap the onGrowthModelUpdated call (and any
external notification/dispatch) behind if (notify) so callers can pass notify:
false during bulk operations, and ensure the branch that removes stale models
also respects notify; update bulk callers to pass notify: false as needed.
Comment on lines
+164
to
+166
| for (var session in sessions) { | ||
| if (session.date.isBefore(weekAgo)) continue; | ||
Contributor
There was a problem hiding this comment.
Exclude future-dated sessions from weekly volume.
If any sessions are dated after now, they’ll incorrectly inflate “last 7 days” stats.
🧭 Filter out future sessions
- for (var session in sessions) {- if (session.date.isBefore(weekAgo)) continue;+ for (var session in sessions) {+ if (session.date.isBefore(weekAgo) || session.date.isAfter(currentTime)) {+ continue;+ }🤖 Prompt for AI Agents
In `@workout-logger/lib/services/managers/analytics_manager.dart` around lines 164
- 166, The loop that filters sessions for the past week currently only excludes
sessions before weekAgo and can include future-dated sessions; inside the same
for-loop that iterates over sessions (the code referencing sessions, weekAgo,
and now in analytics_manager.dart) add a check to also skip sessions with
session.date after now (e.g., if (session.date.isAfter(now)) continue;) so only
sessions with weekAgo <= session.date <= now are counted for weekly volume.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@FIREBASE_TEST_CASES.yaml`:
- Around line 42-45: The successCriteria entries currently hardcode units ("100
kg × 10 reps" and "105 kg × 8 reps"), which breaks when units/locales change;
update the FIREBASE_TEST_CASES.yaml successCriteria fields (the two
successCriteria values associated with the first set and the "Add another set"
step) to be unit-agnostic—either strip unit text (e.g., "100 × 10" / "105 × 8"),
use a more generic phrase ("100 × 10 reps" / "105 × 8 reps"), or replace them
with regex-like patterns that allow kg or lb and localized separators (e.g.,
"\d+\s*(kg|lb)?\s*×\s*\d+" or similar) so the tests aren’t brittle across unit
settings.
- Around line 170-179: The test chain branching causes state conflicts: ensure
each chain that references start_quick_workout (e.g., cancel_active_workout and
log_exercise_sets) executes in isolation by either (1) making
start_quick_workout idempotent and adding an explicit teardown/reset step after
it (clear workout state, return to home) or (2) configuring Firebase Test Lab to
run each prerequisite chain independently (separate test matrices or isolated
test runs for cancel_active_workout and log_exercise_sets) so one chain’s
completion cannot change the other's initial state; update the
FIREBASE_TEST_CASES.yaml entries for cancel_active_workout and log_exercise_sets
to include the chosen setup/teardown or to be executed in isolated runs.
- Around line 69-98: Add three new YAML test cases to cover deletion and editing
flows: create a test case id delete_routine that has prerequisiteTestCaseId:
create_new_routine and steps to open the "Push Day" routine, tap "Delete"
(confirm) and assert the routine no longer appears; add edit_routine with
prerequisiteTestCaseId: create_new_routine that opens the routine, edits the
name/exercises, saves, and asserts the updated details display; and add
delete_history_entry with prerequisiteTestCaseId: edit_workout_history (or
start_workout_from_routine) that navigates to History, selects a workout,
deletes it (confirm) and asserts it is removed; reference RoutineManager and
HistoryManager behaviors in hints/successCriteria to ensure tests exercise the
supported delete/edit operations.
Comment on lines
+42
to
+45
| successCriteria: Set appears in the list showing "100 kg × 10 reps" | ||
| - goal: Add another set | ||
| hint: Enter weight "105" and reps "8", then tap "Add Set" | ||
| successCriteria: Second set appears showing "105 kg × 8 reps" |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Success criteria assumes metric units.
The success criteria hardcode "100 kg × 10 reps" and "105 kg × 8 reps". If the app supports user-configurable units (lbs/kg) or localization, consider making these criteria unit-agnostic (e.g., "100 × 10" or regex patterns) to avoid test brittleness.
🤖 Prompt for AI Agents
In `@FIREBASE_TEST_CASES.yaml` around lines 42 - 45, The successCriteria entries
currently hardcode units ("100 kg × 10 reps" and "105 kg × 8 reps"), which
breaks when units/locales change; update the FIREBASE_TEST_CASES.yaml
successCriteria fields (the two successCriteria values associated with the first
set and the "Add another set" step) to be unit-agnostic—either strip unit text
(e.g., "100 × 10" / "105 × 8"), use a more generic phrase ("100 × 10 reps" /
"105 × 8 reps"), or replace them with regex-like patterns that allow kg or lb
and localized separators (e.g., "\d+\s*(kg|lb)?\s*×\s*\d+" or similar) so the
tests aren’t brittle across unit settings.
Comment on lines
+69
to
+98
| - displayName: Create a New Routine | ||
| id: create_new_routine | ||
| prerequisiteTestCaseId: launch_app | ||
| steps: | ||
| - goal: Navigate to routines screen | ||
| hint: Tap "Routines" tab or button in navigation | ||
| successCriteria: Screen shows list of routines with "Create Routine" or "+" button | ||
| - goal: Start creating a new routine | ||
| hint: Tap "Create Routine" or "+" button | ||
| successCriteria: Routine creation screen appears with name input field | ||
| - goal: Enter routine name | ||
| hint: Tap name field and type "Push Day" | ||
| successCriteria: Name field shows "Push Day" | ||
| - goal: Add exercises to the routine | ||
| hint: Tap "Add Exercises" button and select 3-4 exercises (e.g., Bench Press, Shoulder Press, Tricep Dips) | ||
| successCriteria: Selected exercises appear in the routine exercise list | ||
| - goal: Save the routine | ||
| hint: Tap "Save" button | ||
| successCriteria: Screen returns to routines list showing the new "Push Day" routine | ||
| - displayName: Start Workout from Routine | ||
| id: start_workout_from_routine | ||
| prerequisiteTestCaseId: create_new_routine | ||
| steps: | ||
| - goal: Select a routine to start workout | ||
| hint: On routines screen, tap on "Push Day" routine | ||
| successCriteria: Routine details screen shows exercises in the routine | ||
| - goal: Start workout from this routine | ||
| hint: Tap "Start Workout" button | ||
| successCriteria: Workout flow screen displays with first exercise from the routine |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding test cases for routine and history deletion.
The RoutineManager and HistoryManager support delete operations, but there are no test cases for:
- Deleting a routine
- Editing a routine
- Deleting a workout from history
These would complement the existing coverage for create_new_routine and edit_workout_history.
🤖 Prompt for AI Agents
In `@FIREBASE_TEST_CASES.yaml` around lines 69 - 98, Add three new YAML test cases
to cover deletion and editing flows: create a test case id delete_routine that
has prerequisiteTestCaseId: create_new_routine and steps to open the "Push Day"
routine, tap "Delete" (confirm) and assert the routine no longer appears; add
edit_routine with prerequisiteTestCaseId: create_new_routine that opens the
routine, edits the name/exercises, saves, and asserts the updated details
display; and add delete_history_entry with prerequisiteTestCaseId:
edit_workout_history (or start_workout_from_routine) that navigates to History,
selects a workout, deletes it (confirm) and asserts it is removed; reference
RoutineManager and HistoryManager behaviors in hints/successCriteria to ensure
tests exercise the supported delete/edit operations.
Comment on lines
+170
to
+179
| - displayName: Cancel Active Workout | ||
| id: cancel_active_workout | ||
| prerequisiteTestCaseId: start_quick_workout | ||
| steps: | ||
| - goal: Initiate workout cancellation | ||
| hint: During active workout, tap back button or menu and select "Cancel Workout" | ||
| successCriteria: Confirmation dialog appears warning that progress will not be saved | ||
| - goal: Confirm cancellation | ||
| hint: Tap "Cancel Workout" in the confirmation dialog | ||
| successCriteria: Returns to home screen without saving, workout count unchanged |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider test isolation for branching prerequisite chains.
Both cancel_active_workout and log_exercise_sets depend on start_quick_workout, creating a branching execution path. Ensure your Firebase Test Lab configuration runs each test chain independently to avoid state conflicts (e.g., one test completing the workout while another tries to cancel it).
🤖 Prompt for AI Agents
In `@FIREBASE_TEST_CASES.yaml` around lines 170 - 179, The test chain branching
causes state conflicts: ensure each chain that references start_quick_workout
(e.g., cancel_active_workout and log_exercise_sets) executes in isolation by
either (1) making start_quick_workout idempotent and adding an explicit
teardown/reset step after it (clear workout state, return to home) or (2)
configuring Firebase Test Lab to run each prerequisite chain independently
(separate test matrices or isolated test runs for cancel_active_workout and
log_exercise_sets) so one chain’s completion cannot change the other's initial
state; update the FIREBASE_TEST_CASES.yaml entries for cancel_active_workout and
log_exercise_sets to include the chosen setup/teardown or to be executed in
isolated runs.
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Mar 19, 2026
This was referenced Apr 25, 2026
This was referenced May 2, 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 freeto 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.
feat: Create RoutineManager for managing workout routines
feat: Develop TargetManager for managing workout targets/goals
refactor: Update MLService to implement IMLService interface
refactor: Implement StorageService with IStorageService interface
feat: Introduce TargetCalculator strategy pattern for target value calculations
refactor: Update WorkoutProvider to use individual managers
test: Add mock services for ML and storage for testing
Summary by CodeRabbit
Documentation
New Features
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.