Add Training Program feature with manager and UI rename - #27
Closed
Devasy wants to merge 13 commits into
Closed
Conversation
Documents project structure, architecture patterns (SOLID), data models, development commands, testing conventions, CI/CD pipeline, theme system, and key conventions for AI assistants working on the codebase. https://claude.ai/code/session_01NytaxeQoLadhKcESsKqjmT
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Adds a complete multi-week training program feature inspired by the 12-week periodisation reference plan (phases, deload weeks, tempo notation, rep ranges, rest times, superset grouping). Models (models.dart): - ProgramExerciseSlot — per-exercise parameters (sets, rep range, rest seconds, tempo, weight %, notes, superset group) - ProgramDay — named training day with ordered exercise slots - ProgramWeek — week with deload flag, intensity factor, set reduction - TrainingPhase — named phase spanning a range of weeks - TrainingProgram — top-level entity; full JSON serialization Storage: - New 'training_programs' Hive box in StorageService - saveTrainingProgram / getAllTrainingPrograms / getTrainingProgram / deleteTrainingProgram added to IStorageService interface and implemented in StorageService + MockStorageService ProgramManager (SRP manager): - loadPrograms, saveProgram, createProgram, deleteProgram - importFromJson (assigns new UUID, marks isImported=true) - exportToJson (pretty-printed) WorkoutProvider: - Exposes programManager; loadPrograms() called during init UI (screens/programs/): - ProgramsScreen — list with mini phase timeline, import FAB - ProgramDetailScreen — phase timeline bar, expandable week list with deload badges, per-day exercise breakdown (sets×reps, rest, tempo chip, weight %, superset bracket, notes) - ProgramDesignerScreen — 3-step wizard: metadata/phases → week structure (deload toggle, intensity/set-reduction steppers, add days) → exercise slots per day with full parameter editing RoutinesScreen: - Added Programs tab (TabBar + TabBarView) alongside existing Routines tab; no existing functionality changed https://claude.ai/code/session_01E15CGTZZDU8rz7PX1nYXA3
Contributor
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
WalkthroughAdds a Training Program feature: new immutable program models, storage and Hive persistence, a ProgramManager, program UI (list, designer, detail, import), WorkoutFlow program-mode support, provider wiring, tests/mocks, and CI + repo docs. Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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
No open human review comments were found in this PR to create a plan for. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLAUDE.md`:
- Around line 41-45: Update the inventory text to reflect the new "programs"
feature: change the lib/screens/ count to include the three new program screens
(increase the UI screens count accordingly), add the three program screen
entries into the screens listing (the section that previously omitted them), and
update the summary counts where managers and Hive boxes are listed (adjust the
"managers" and "Hive boxes" counts to their new totals). Locate the sections
that reference "lib/screens/", the "managers" summary block, and the "Hive
boxes" summary and edit those lines so the authoritative counts and the screens
listing include the new program screens.
- Around line 247-256: Update the example Exercise snippet so the id uses a UUID
generator instead of a hardcoded string; replace the 'id' example value
('unique_id') with a call pattern using const Uuid().v4() and add a short note
near the Exercise example that contributors must use const Uuid().v4() (never
sequential integers) when creating new Exercise entries; reference the Exercise
constructor and the id field in the example so readers know exactly which field
to change.
- Around line 193-200: The example uses WorkoutProvider with a positional mockML
but the constructor expects mlService as a named parameter; update the snippet
to call WorkoutProvider with the same positional mockStorage and pass the ML
service using the named parameter mlService (e.g., instantiate
WorkoutProvider(mockStorage, mlService: mockML)) so it matches the constructor
signature in workout-logger/lib/services/workout_provider.dart (WorkoutProvider
and its mlService parameter).
In `@workout-logger/lib/models/models.dart`:
- Around line 488-494: Constructors for ProgramDay, ProgramWeek, and
TrainingProgram currently assign mutable lists directly (e.g.,
ProgramDay.exercises, ProgramWeek.days, TrainingProgram.phases/weeks); change
each constructor to defensively copy and freeze incoming collections (use
List.unmodifiable or a defensive List.from(...).cast<T>() wrapped with
List.unmodifiable) so stored fields are immutable, and ensure any
factory/fromJson paths do the same; update the respective constructors and any
assignment points to wrap collections before assigning to the fields.
In `@workout-logger/lib/screens/programs/program_designer_screen.dart`:
- Around line 809-818: The constructor call for TrainingProgram is forcing
isImported: false which wipes the imported flag on edits; change the assignment
to reuse the existing value when editing by setting isImported to
widget.existing?.isImported ?? false (i.e., keep widget.existing.isImported if
widget.existing is non-null, otherwise default to false) so the imported
badge/behavior is preserved on saves; update the isImported field in the
TrainingProgram creation in program_designer_screen.dart accordingly.
- Around line 715-724: The new ProgramExerciseSlot construction drops
supersetGroupId, which strips edited superset members; preserve the previous
slot's supersetGroupId when rebuilding the slot (e.g., pass supersetGroupId:
existingSlot.supersetGroupId or the editing slot's supersetGroupId into the
ProgramExerciseSlot constructor). If you want full UX, also surface a field in
the edit dialog to create/edit supersetGroupId, but at minimum carry over the
old supersetGroupId value when creating the new ProgramExerciseSlot.
- Around line 895-918: The closures passed to IconButton for increment/decrement
use (value ± step).clamp(min, max) which returns a num but
_NumberStepper.onChanged requires an int; replace those clamp expressions with
manual int-preserving clamping: compute the new int value (e.g., newValue =
value + step or value - step), then if newValue > max set newValue = max, if
newValue < min set newValue = min, and pass that int into onChanged. Update the
two onPressed closures (the decrement and increment handlers) to perform this
int math and clamping before calling onChanged so the type matches
_NumberStepper.onChanged.
In `@workout-logger/lib/screens/programs/program_detail_screen.dart`:
- Around line 454-459: The current map-based grouping (supersetGroups) reorders
exercises; instead iterate day.exercises in order and build contiguous runs:
keep a current buffer and currentGroupId (slot.supersetGroupId), push slots into
the buffer while the groupId stays the same, and when it changes flush the
buffer as either a standalone run (for null) or a superset run (for a non-null
id) into a list of runs; replace uses of supersetGroups with this ordered list
of runs (reference symbols: supersetGroups, slot, day.exercises,
slot.supersetGroupId) and apply the same change to the other block around
506-517.
In `@workout-logger/lib/screens/programs/programs_screen.dart`:
- Around line 206-224: The catch block currently calls Navigator.pop(context)
which closes the bottom sheet even on validation errors; stop popping the sheet
on failure so the user can correct the JSON. In the try/catch around
provider.programManager.importFromJson in programs_screen.dart, remove or guard
the Navigator.pop(context) inside the catch branch (keep the success-path pop in
the try branch), still show the SnackBar with _friendlyError(e) and
AppTheme.error when context.mounted; this keeps the dialog open on import
validation failures.
- Around line 154-156: The UI text incorrectly instructs users to include a
non-existent "totalWeeks" field; update the strings in programs_screen.dart (the
two Text widgets that mention the import JSON shape) to reflect the actual
TrainingProgram serialization in models.dart by removing "totalWeeks" and
listing the real fields (e.g., "name, phases, weeks") so import/export
round-tripping matches the TrainingProgram model.
In `@workout-logger/lib/services/interfaces/storage_service_interface.dart`:
- Around line 63-68: The IStorageService interface currently forces all storage
consumers to implement TrainingProgram CRUD; extract those methods into a
dedicated repository interface (e.g., ITrainingProgramRepository) that declares
Future<void> saveTrainingProgram(TrainingProgram), Future<List<TrainingProgram>>
getAllTrainingPrograms(), Future<TrainingProgram?> getTrainingProgram(String
id), and Future<void> deleteTrainingProgram(String id). Remove these four
methods from IStorageService, make ProgramManager depend on the new
ITrainingProgramRepository instead of IStorageService, and update concrete
storage implementations and mocks to implement ITrainingProgramRepository only
when they actually manage TrainingProgram data. Ensure existing callers are
updated to accept/inject the new interface name (ITrainingProgramRepository) and
run tests to confirm no missing implementations.
In `@workout-logger/lib/services/managers/program_manager.dart`:
- Around line 77-88: The importFromJson method can throw raw cast/type errors
from jsonDecode or TrainingProgram.fromJson; wrap the parsing and model
construction (the jsonDecode(...) cast and TrainingProgram.fromJson(raw) call)
in a try/catch that catches FormatException, TypeError, CastError, and any other
parsing-related errors and rethrow a single FormatException with a clear message
(including the original error.message) before exiting the method; ensure you
still assign new id/isImported/createdAt and only call saveProgram(program) when
parsing succeeds, preserving use of _uuid.v4(), TrainingProgram.fromJson, and
saveProgram.
In `@workout-logger/lib/services/storage_service.dart`:
- Around line 299-327: Export/import currently omit the
`_trainingProgramsBoxInstance`, causing programs to be lost; update
`exportAllData()` to include all entries from `_trainingProgramsBoxInstance`
(serialize each `TrainingProgram` via `toJson()`/`jsonEncode` or as JSON objects
under a `trainingPrograms` key) and update `importData()` to read that
`trainingPrograms` payload and repopulate the box (use the same insertion format
as `saveTrainingProgram` and `TrainingProgram.fromJson()` for each item), making
sure to handle clearing or merging existing entries consistently and keep the
payload shape/versioning consistent with other exported sections.
In `@workout-logger/lib/services/workout_provider.dart`:
- Around line 67-70: WorkoutProvider currently constructs ProgramManager
internally (programManager = ProgramManager(_storage)) which prevents swapping
fakes; change WorkoutProvider to accept a ProgramManager in its constructor (add
a ProgramManager parameter and assign to the programManager field, removing the
internal instantiation), update AppInitializer (composition root, e.g. in
main.dart) to create ProgramManager(_storage) and pass it into the
WorkoutProvider constructor, and update tests to inject a fake ProgramManager
when needed; keep the existing _storage and _mlService wiring unchanged.
In `@workout-logger/test/test_utils/mock_storage_service.dart`:
- Around line 239-241: The mock getAllTrainingPrograms should return programs in
the same newest-first order as production: instead of returning
List.from(_trainingPrograms) preserve _trainingPrograms and return a sorted copy
ordered by createdAt descending (newest first); update the
getAllTrainingPrograms implementation to create a new list from
_trainingPrograms and sort it by each TrainingProgram.createdAt (or
createdAtUtc) in descending order so tests see the same ordering as the real
StorageService.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 75653222-bdf4-4f2e-9c31-35826999c543
📒 Files selected for processing (12)
CLAUDE.mdworkout-logger/lib/models/models.dartworkout-logger/lib/screens/programs/program_designer_screen.dartworkout-logger/lib/screens/programs/program_detail_screen.dartworkout-logger/lib/screens/programs/programs_screen.dartworkout-logger/lib/screens/routines_screen.dartworkout-logger/lib/services/interfaces/storage_service_interface.dartworkout-logger/lib/services/managers/managers.dartworkout-logger/lib/services/managers/program_manager.dartworkout-logger/lib/services/storage_service.dartworkout-logger/lib/services/workout_provider.dartworkout-logger/test/test_utils/mock_storage_service.dart
Comment on lines
+41
to
+45
| │ │ ├── screens/ # 7 UI screens | ||
| │ │ └── data/ | ||
| │ │ └── exercise_database.dart # 50+ built-in exercises | ||
| │ ├── test/ # flutter_test + Mockito tests | ||
| │ │ └── test_utils/ # MockStorageService, MockMLService |
Contributor
There was a problem hiding this comment.
Refresh the inventory sections for the new programs feature.
Line 41 still says lib/screens/ contains 7 UI screens, Line 105 still lists 6 managers, Line 119 still lists 6 Hive boxes, and Lines 175-181 omit the three new program screens. Since this file is meant to be authoritative, those sections now point assistants at the pre-program architecture.
Also applies to: 103-109, 117-121, 173-181
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLAUDE.md` around lines 41 - 45, Update the inventory text to reflect the new
"programs" feature: change the lib/screens/ count to include the three new
program screens (increase the UI screens count accordingly), add the three
program screen entries into the screens listing (the section that previously
omitted them), and update the summary counts where managers and Hive boxes are
listed (adjust the "managers" and "Hive boxes" counts to their new totals).
Locate the sections that reference "lib/screens/", the "managers" summary block,
and the "Hive boxes" summary and edit those lines so the authoritative counts
and the screens listing include the new program screens.
Uh oh!
There was an error while loading. Please reload this page.
Comment on lines
+247
to
+256
| ```dart | ||
| Exercise( | ||
| id: 'unique_id', | ||
| name: 'Exercise Name', | ||
| category: 'compound', // or 'isolation' | ||
| muscleActivations: [ | ||
| MuscleActivation(muscleGroupId: 'chest', activationPercentage: 70), | ||
| MuscleActivation(muscleGroupId: 'triceps', activationPercentage: 30), | ||
| ], | ||
| ), |
Contributor
There was a problem hiding this comment.
Document UUID generation for new exercises.
Line 249’s 'unique_id' example contradicts the repo convention for exercise IDs and will encourage collisions in future additions.
Suggested doc fix
Exercise(
- id: 'unique_id',+ id: const Uuid().v4(),
name: 'Exercise Name',📝 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
| ```dart | |
| Exercise( | |
| id: 'unique_id', | |
| name: 'Exercise Name', | |
| category: 'compound', // or 'isolation' | |
| muscleActivations: [ | |
| MuscleActivation(muscleGroupId: 'chest', activationPercentage: 70), | |
| MuscleActivation(muscleGroupId: 'triceps', activationPercentage: 30), | |
| ], | |
| ), |
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 247-247: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLAUDE.md` around lines 247 - 256, Update the example Exercise snippet so the
id uses a UUID generator instead of a hardcoded string; replace the 'id' example
value ('unique_id') with a call pattern using const Uuid().v4() and add a short
note near the Exercise example that contributors must use const Uuid().v4()
(never sequential integers) when creating new Exercise entries; reference the
Exercise constructor and the id field in the example so readers know exactly
which field to change.
Comment on lines
+488
to
+494
| ProgramDay({ | ||
| required this.id, | ||
| required this.name, | ||
| this.dayOfWeek, | ||
| this.notes, | ||
| required this.exercises, | ||
| }); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Freeze list fields before exposing these models.
ProgramDay.exercises, ProgramWeek.days, and TrainingProgram.phases / weeks are all stored and returned as mutable lists. That lets callers mutate program state in place and bypass the immutable-update flow that copyWith() and the managers are relying on. Wrap incoming collections with defensive copies or List.unmodifiable in these constructors so every path stays immutable.
As per coding guidelines, "All data models must implement copyWith() for immutable updates; never mutate state directly".
Also applies to: 545-553, 669-679
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workout-logger/lib/models/models.dart` around lines 488 - 494, Constructors
for ProgramDay, ProgramWeek, and TrainingProgram currently assign mutable lists
directly (e.g., ProgramDay.exercises, ProgramWeek.days,
TrainingProgram.phases/weeks); change each constructor to defensively copy and
freeze incoming collections (use List.unmodifiable or a defensive
List.from(...).cast<T>() wrapped with List.unmodifiable) so stored fields are
immutable, and ensure any factory/fromJson paths do the same; update the
respective constructors and any assignment points to wrap collections before
assigning to the fields.
Uh oh!
There was an error while loading. Please reload this page.
Comment on lines
+63
to
+68
| // ==================== TRAINING PROGRAMS ==================== | ||
| Future<void> saveTrainingProgram(TrainingProgram program); | ||
| Future<List<TrainingProgram>> getAllTrainingPrograms(); | ||
| Future<TrainingProgram?> getTrainingProgram(String id); | ||
| Future<void> deleteTrainingProgram(String id); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Split program persistence out of IStorageService.
Lines 65-68 make every storage consumer and every mock implement training-program CRUD even when they never touch programs. ProgramManager only needs a narrow repository here, so this feature is deepening the monolithic storage contract instead of moving toward smaller interfaces.
Based on learnings: StorageService acts as a monolithic repository for all data types, and the recommended approach is to extract interfaces like IStorageService and IExerciseRepository to allow different storage backends without modifying consumers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workout-logger/lib/services/interfaces/storage_service_interface.dart` around
lines 63 - 68, The IStorageService interface currently forces all storage
consumers to implement TrainingProgram CRUD; extract those methods into a
dedicated repository interface (e.g., ITrainingProgramRepository) that declares
Future<void> saveTrainingProgram(TrainingProgram), Future<List<TrainingProgram>>
getAllTrainingPrograms(), Future<TrainingProgram?> getTrainingProgram(String
id), and Future<void> deleteTrainingProgram(String id). Remove these four
methods from IStorageService, make ProgramManager depend on the new
ITrainingProgramRepository instead of IStorageService, and update concrete
storage implementations and mocks to implement ITrainingProgramRepository only
when they actually manage TrainingProgram data. Ensure existing callers are
updated to accept/inject the new interface name (ITrainingProgramRepository) and
run tests to confirm no missing implementations.
Comment on lines
+77
to
+88
| Future<TrainingProgram> importFromJson(String jsonString) async { | ||
| final Map<String, dynamic> raw = | ||
| jsonDecode(jsonString) as Map<String, dynamic>; | ||
| // Assign a new local ID and mark as imported | ||
| raw['id'] = _uuid.v4(); | ||
| raw['isImported'] = true; | ||
| raw['createdAt'] = DateTime.now().toIso8601String(); | ||
| final program = TrainingProgram.fromJson(raw); | ||
| await saveProgram(program); | ||
| return program; |
Contributor
There was a problem hiding this comment.
Normalize import failures to FormatException.
jsonDecode(... ) as Map<String, dynamic> and TrainingProgram.fromJson can currently throw raw cast/type errors on malformed user input, even though this method promises FormatException. Catch parse/schema failures here and rethrow a single FormatException so the import flow can handle bad JSON predictably.
Possible fix
Future<TrainingProgram> importFromJson(String jsonString) async {
- final Map<String, dynamic> raw =- jsonDecode(jsonString) as Map<String, dynamic>;-- // Assign a new local ID and mark as imported- raw['id'] = _uuid.v4();- raw['isImported'] = true;- raw['createdAt'] = DateTime.now().toIso8601String();-- final program = TrainingProgram.fromJson(raw);- await saveProgram(program);- return program;+ try {+ final decoded = jsonDecode(jsonString);+ if (decoded is! Map<String, dynamic>) {+ throw const FormatException('Program JSON must be an object.');+ }++ final raw = Map<String, dynamic>.from(decoded);+ raw['id'] = _uuid.v4();+ raw['isImported'] = true;+ raw['createdAt'] = DateTime.now().toIso8601String();++ final program = TrainingProgram.fromJson(raw);+ await saveProgram(program);+ return program;+ } on FormatException {+ rethrow;+ } catch (e) {+ throw FormatException('Invalid training program JSON: $e');+ }
}📝 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
| Future<TrainingProgram> importFromJson(String jsonString) async { | |
| finalMap<String, dynamic> raw = | |
| jsonDecode(jsonString) asMap<String, dynamic>; | |
| // Assign a new local ID and mark as imported | |
| raw['id'] = _uuid.v4(); | |
| raw['isImported'] =true; | |
| raw['createdAt'] =DateTime.now().toIso8601String(); | |
| final program =TrainingProgram.fromJson(raw); | |
| awaitsaveProgram(program); | |
| return program; | |
| Future<TrainingProgram> importFromJson(String jsonString) async { | |
| try { | |
| final decoded =jsonDecode(jsonString); | |
| if (decoded is!Map<String, dynamic>) { | |
| throwconstFormatException('Program JSON must be an object.'); | |
| } | |
| final raw =Map<String, dynamic>.from(decoded); | |
| raw['id'] = _uuid.v4(); | |
| raw['isImported'] =true; | |
| raw['createdAt'] =DateTime.now().toIso8601String(); | |
| final program =TrainingProgram.fromJson(raw); | |
| awaitsaveProgram(program); | |
| return program; | |
| } onFormatException { | |
| rethrow; | |
| } catch (e) { | |
| throwFormatException('Invalid training program JSON: $e'); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workout-logger/lib/services/managers/program_manager.dart` around lines 77 -
88, The importFromJson method can throw raw cast/type errors from jsonDecode or
TrainingProgram.fromJson; wrap the parsing and model construction (the
jsonDecode(...) cast and TrainingProgram.fromJson(raw) call) in a try/catch that
catches FormatException, TypeError, CastError, and any other parsing-related
errors and rethrow a single FormatException with a clear message (including the
original error.message) before exiting the method; ensure you still assign new
id/isImported/createdAt and only call saveProgram(program) when parsing
succeeds, preserving use of _uuid.v4(), TrainingProgram.fromJson, and
saveProgram.
Comment on lines
+299
to
+327
| @override | ||
| Future<void> saveTrainingProgram(TrainingProgram program) async { | ||
| await _trainingProgramsBoxInstance.put( | ||
| program.id, | ||
| jsonEncode(program.toJson()), | ||
| ); | ||
| } | ||
| @override | ||
| Future<List<TrainingProgram>> getAllTrainingPrograms() async { | ||
| final programs = <TrainingProgram>[]; | ||
| for (final json in _trainingProgramsBoxInstance.values) { | ||
| programs.add(TrainingProgram.fromJson(jsonDecode(json))); | ||
| } | ||
| programs.sort((a, b) => b.createdAt.compareTo(a.createdAt)); | ||
| return programs; | ||
| } | ||
| @override | ||
| Future<TrainingProgram?> getTrainingProgram(String id) async { | ||
| final json = _trainingProgramsBoxInstance.get(id); | ||
| if (json == null) return null; | ||
| return TrainingProgram.fromJson(jsonDecode(json)); | ||
| } | ||
| @override | ||
| Future<void> deleteTrainingProgram(String id) async { | ||
| await _trainingProgramsBoxInstance.delete(id); | ||
| } |
Contributor
There was a problem hiding this comment.
Include training programs in full-app export/import.
Now that programs are stored in _trainingProgramsBoxInstance, exportAllData() and importData() above still ignore that box. A user who exports app data and restores it later will silently lose every training program.
Suggested fix
final data = {
'sessions': _sessionsBox.values.toList(),
'routines': _routinesBoxInstance.values.toList(),
'targets': _targetsBoxInstance.values.toList(),
'muscleGroups': _muscleGroupsBoxInstance.values.toList(),
'customExercises': _customExercisesBoxInstance.values.toList(),
+ 'trainingPrograms': _trainingProgramsBoxInstance.values.toList(),
'exportDate': DateTime.now().toIso8601String(),
}; // Import targets
if (data['targets'] != null) {
for (var json in data['targets']) {
final target = Target.fromJson(jsonDecode(json));
await saveTarget(target);
}
}
++ // Import training programs+ if (data['trainingPrograms'] != null) {+ for (final json in data['trainingPrograms']) {+ final program = TrainingProgram.fromJson(jsonDecode(json));+ await saveTrainingProgram(program);+ }+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workout-logger/lib/services/storage_service.dart` around lines 299 - 327,
Export/import currently omit the `_trainingProgramsBoxInstance`, causing
programs to be lost; update `exportAllData()` to include all entries from
`_trainingProgramsBoxInstance` (serialize each `TrainingProgram` via
`toJson()`/`jsonEncode` or as JSON objects under a `trainingPrograms` key) and
update `importData()` to read that `trainingPrograms` payload and repopulate the
box (use the same insertion format as `saveTrainingProgram` and
`TrainingProgram.fromJson()` for each item), making sure to handle clearing or
merging existing entries consistently and keep the payload shape/versioning
consistent with other exported sections.
Uh oh!
There was an error while loading. Please reload this page.
Comment on lines
+239
to
+241
| @override | ||
| Future<List<TrainingProgram>> getAllTrainingPrograms() async => | ||
| List.from(_trainingPrograms); |
Contributor
There was a problem hiding this comment.
Return training programs in the same order as production.
Line 240 currently returns insertion order, but workout-logger/lib/services/storage_service.dart:307-315 sorts newest-first by createdAt. Tests that exercise list ordering will now see different behavior from the real app.
Suggested fix
`@override`
Future<List<TrainingProgram>> getAllTrainingPrograms() async =>
- List.from(_trainingPrograms);+ List<TrainingProgram>.from(_trainingPrograms)+ ..sort((a, b) => b.createdAt.compareTo(a.createdAt));📝 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
| @override | |
| Future<List<TrainingProgram>> getAllTrainingPrograms() async=> | |
| List.from(_trainingPrograms); | |
| `@override` | |
| Future<List<TrainingProgram>> getAllTrainingPrograms() async=> | |
| List<TrainingProgram>.from(_trainingPrograms) | |
| ..sort((a, b) => b.createdAt.compareTo(a.createdAt)); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workout-logger/test/test_utils/mock_storage_service.dart` around lines 239 -
241, The mock getAllTrainingPrograms should return programs in the same
newest-first order as production: instead of returning
List.from(_trainingPrograms) preserve _trainingPrograms and return a sorted copy
ordered by createdAt descending (newest first); update the
getAllTrainingPrograms implementation to create a new list from
_trainingPrograms and sort it by each TrainingProgram.createdAt (or
createdAtUtc) in descending order so tests see the same ordering as the real
StorageService.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
workout-logger/lib/services/workout_provider.dart (1)
67-70: 🛠️ Refactor suggestion | 🟠 MajorInject
ProgramManagerinstead of constructing it inWorkoutProvider.Internal construction at Line 69 pulls object-graph wiring into this provider and reduces substitutability in tests.
Possible refactor
- late final ProgramManager programManager;+ final ProgramManager programManager;- WorkoutProvider(this._storage, {IMLService? mlService})- : _mlService = mlService ?? MLService() {- programManager = ProgramManager(_storage);- }+ WorkoutProvider(+ this._storage, {+ required this.programManager,+ IMLService? mlService,+ }) : _mlService = mlService ?? MLService();Based on learnings: Use
AppInitializerinmain.dartas the composition root for all dependency injection, wiring services via constructor injection intoWorkoutProvider.🤖 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 67 - 70, WorkoutProvider currently instantiates ProgramManager in its constructor (programManager = ProgramManager(_storage)), which couples wiring and hinders testing; change WorkoutProvider to accept a ProgramManager via constructor injection (add a ProgramManager parameter, e.g., WorkoutProvider(this._storage, {IMLService? mlService, required ProgramManager programManager}) and assign it to the programManager field) and remove the internal new ProgramManager(...) call; wire up the ProgramManager from your composition root (AppInitializer/main) when constructing WorkoutProvider so tests can provide mocks or fakes.workout-logger/lib/services/storage_service.dart (1)
342-361:⚠️ Potential issue | 🟠 MajorInclude training programs in full export/import to prevent silent data loss.
Line 342 and Line 366 still omit
trainingPrograms, so backups won’t restore program data even though it is now persisted.Suggested fix
final data = { 'sessions': _sessionsBox.values .map(_normalizeExportValue) .toList(growable: false), 'routines': _routinesBoxInstance.values .map(_normalizeExportValue) .toList(growable: false), 'targets': _targetsBoxInstance.values .map(_normalizeExportValue) .toList(growable: false), 'muscleGroups': _muscleGroupsBoxInstance.values .map(_normalizeExportValue) .toList(growable: false), 'customExercises': _customExercisesBoxInstance.values .map(_normalizeExportValue) .toList(growable: false), + 'trainingPrograms': _trainingProgramsBoxInstance.values+ .map(_normalizeExportValue)+ .toList(growable: false), 'settings': settingsMap, 'exportDate': DateTime.now().toIso8601String(), 'appVersion': _appVersion, };+ // Import training programs (merge: skip if id already exists)+ final trainingPrograms = data['trainingPrograms'];+ if (trainingPrograms is List) {+ for (final item in trainingPrograms) {+ final map = _normalizeImportItem(item);+ if (map == null) continue;+ final program = TrainingProgram.fromJson(map);+ final existing = await getTrainingProgram(program.id);+ if (existing == null) {+ await saveTrainingProgram(program);+ }+ }+ }Also applies to: 366-449
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workout-logger/lib/services/storage_service.dart` around lines 342 - 361, The export/import data structures currently omit trainingPrograms causing silent data loss; update the export payload (the Map built into the variable named data) to include a 'trainingPrograms' key whose value is _trainingProgramsBoxInstance.values.map(_normalizeExportValue).toList(growable: false), and then update the corresponding import/restore logic that reads this payload (the code that iterates over sessions/routines/targets/etc. during import) to also parse and restore 'trainingPrograms' entries using the same deserialization/normalization helpers and box insertion used for other models (reuse _normalizeExportValue and whatever insert/put methods are used for _routinesBoxInstance/_customExercisesBoxInstance). Ensure both export and import use the same key name 'trainingPrograms' so backups round-trip without data loss.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@workout-logger/lib/services/storage_service.dart`:
- Around line 342-361: The export/import data structures currently omit
trainingPrograms causing silent data loss; update the export payload (the Map
built into the variable named data) to include a 'trainingPrograms' key whose
value is
_trainingProgramsBoxInstance.values.map(_normalizeExportValue).toList(growable:
false), and then update the corresponding import/restore logic that reads this
payload (the code that iterates over sessions/routines/targets/etc. during
import) to also parse and restore 'trainingPrograms' entries using the same
deserialization/normalization helpers and box insertion used for other models
(reuse _normalizeExportValue and whatever insert/put methods are used for
_routinesBoxInstance/_customExercisesBoxInstance). Ensure both export and import
use the same key name 'trainingPrograms' so backups round-trip without data
loss.
In `@workout-logger/lib/services/workout_provider.dart`:
- Around line 67-70: WorkoutProvider currently instantiates ProgramManager in
its constructor (programManager = ProgramManager(_storage)), which couples
wiring and hinders testing; change WorkoutProvider to accept a ProgramManager
via constructor injection (add a ProgramManager parameter, e.g.,
WorkoutProvider(this._storage, {IMLService? mlService, required ProgramManager
programManager}) and assign it to the programManager field) and remove the
internal new ProgramManager(...) call; wire up the ProgramManager from your
composition root (AppInitializer/main) when constructing WorkoutProvider so
tests can provide mocks or fakes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 919defbc-5b63-4cfa-af4f-d2275f21eb19
📒 Files selected for processing (2)
workout-logger/lib/services/storage_service.dartworkout-logger/lib/services/workout_provider.dart
example_12_week_program.json (docs/): - 12 weeks · 3 phases (Foundation W1–4, Intensify W5–8, Peak W9–12) - 5 training days/week: Push (Mon), Core (Wed), Pull (Thu), Shoulders & Arms (Fri), Legs + Rehab (Sat) - 344 exercise slots across 60 days (~98 KB) - Deload weeks 5 (85% / −1 set) and 9 (90% / −1 set) - Supersets encoded with supersetGroupId per phase: ss-push-a (tricep pushdown + lateral raise, W5+) ss-pull-a (preacher curl + hammer curl, W5+) ss-shld-a (lateral raise + rear delt fly, W5+) ss-arm-a (bicep curl + tricep pushdown, W5+) - All exercise IDs match built-in ExerciseDatabase entries - Tempo notation on all compound lifts (e.g. "3-1-1") ImportProgramScreen (screens/programs/import_program_screen.dart): - Full-screen page replaces the cramped 8-line bottom sheet - Expands text area fills available viewport height (maxLines: null) - Live char / line / KB counter in status bar - Two-step flow: Validate → Import (button disabled until valid) - Inline field-level error messages (missing key, wrong type) - Green border + "Valid JSON" indicator on success - Red border + inline error on failure - Clear (×) button in AppBar ProgramsScreen: - _showImportDialog / _doImport / _friendlyError removed - Replaced with _openImport → Navigator.push to ImportProgramScreen - Success snackbar shown on return with result == true https://claude.ai/code/session_01E15CGTZZDU8rz7PX1nYXA3
file_picker (already in pubspec) is now wired into the import screen: - Folder icon in AppBar opens native file picker filtered to .json - Selected file content is read and loaded into the text field - Validation / Import flow unchanged after loading Instructions banner updated to mention both input methods. https://claude.ai/code/session_01E15CGTZZDU8rz7PX1nYXA3
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/import_program_screen.dart`:
- Around line 142-144: The empty setState(() {}) used to refresh the character
counter is unnecessary; replace that manual refresh by wrapping the status bar /
char counter widget with a ValueListenableBuilder<TextEditingValue> that listens
to the existing TextEditingController _ctrl so the counter rebuilds
automatically on text changes, and remove the empty setState call from wherever
it's invoked (leave validation-related setState logic intact) so the character
display is decoupled from validation updates.
- Around line 278-289: The current shallow validation via _requireField lets
malformed JSON slip through because nested structures (phases and weeks.days)
required by TrainingProgram.fromJson aren't checked; update the Validate step to
perform a dry-run parse by calling TrainingProgram.fromJson(decoded) (or
otherwise deep-validate phases and weeks structure) and catch/propagate any
exceptions so users see real parse errors before import; alternatively, if you
keep it shallow, update the UI/text to state the validation is shallow and
actual import (in _import()) may still fail.
In `@workout-logger/lib/screens/programs/programs_screen.dart`:
- Around line 271-293: The mini-timeline calculation in _buildMiniTimeline uses
program.totalWeeks directly causing division-by-zero if totalWeeks == 0; guard
by checking program.totalWeeks (or compute a safeDenominator =
program.totalWeeks > 0 ? program.totalWeeks : 1) before computing fraction, or
skip/return an empty/safe widget when totalWeeks is 0, then compute fraction =
(phase.endWeek - phase.startWeek + 1) / safeDenominator and continue using that
fraction when building the Expanded flex so no Infinity/NaN values are produced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5f81f99d-1759-4f4c-b7ad-e90ee89414ed
📒 Files selected for processing (3)
docs/example_12_week_program.jsonworkout-logger/lib/screens/programs/import_program_screen.dartworkout-logger/lib/screens/programs/programs_screen.dart
Comment on lines
+142
to
+144
| } else { | ||
| setState(() {}); // refresh char counter | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider ValueListenableBuilder to avoid empty setState.
The empty setState(() {}) on Line 143 works but is slightly unusual. Using ValueListenableBuilder with _ctrl would automatically rebuild on text changes without manual state management.
Alternative approach
Wrap the status bar in a ValueListenableBuilder<TextEditingValue> listening to _ctrl to decouple the character counter from validation state updates.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workout-logger/lib/screens/programs/import_program_screen.dart` around lines
142 - 144, The empty setState(() {}) used to refresh the character counter is
unnecessary; replace that manual refresh by wrapping the status bar / char
counter widget with a ValueListenableBuilder<TextEditingValue> that listens to
the existing TextEditingController _ctrl so the counter rebuilds automatically
on text changes, and remove the empty setState call from wherever it's invoked
(leave validation-related setState logic intact) so the character display is
decoupled from validation updates.
Comment on lines
+278
to
+289
| // Required field checks | ||
| _requireField(decoded, 'name', String); | ||
| _requireField(decoded, 'totalWeeks', int); | ||
| _requireField(decoded, 'phases', List); | ||
| _requireField(decoded, 'weeks', List); | ||
| // Light structural check on first week | ||
| if ((decoded['weeks'] as List).isNotEmpty) { | ||
| final w = (decoded['weeks'] as List).first as Map<String, dynamic>; | ||
| _requireField(w, 'weekNumber', int); | ||
| _requireField(w, 'days', List); | ||
| } |
Contributor
There was a problem hiding this comment.
Validation passes JSON that will fail during actual import.
The validation checks top-level fields (name, totalWeeks, phases, weeks) and only samples weeks[0], but TrainingProgram.fromJson requires deeply nested structures in phases (each needs id, name, startWeek, endWeek) and weeks[*].days[*]. A user could paste JSON that passes validation but fails on import with a cryptic type-cast error.
Consider either:
- Delegating full validation to
fromJsonby attempting a dry-run parse, or - Documenting that "Validate" is a shallow check and the import may still fail.
Since _import() already catches and surfaces errors gracefully (Line 338-346), this is acceptable but the UX could mislead users into thinking validated JSON is import-ready.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workout-logger/lib/screens/programs/import_program_screen.dart` around lines
278 - 289, The current shallow validation via _requireField lets malformed JSON
slip through because nested structures (phases and weeks.days) required by
TrainingProgram.fromJson aren't checked; update the Validate step to perform a
dry-run parse by calling TrainingProgram.fromJson(decoded) (or otherwise
deep-validate phases and weeks structure) and catch/propagate any exceptions so
users see real parse errors before import; alternatively, if you keep it
shallow, update the UI/text to state the validation is shallow and actual import
(in _import()) may still fail.
Comment on lines
+271
to
+293
| Widget _buildMiniTimeline() { | ||
| return SizedBox( | ||
| height: 4, | ||
| child: Row( | ||
| children: program.phases.asMap().entries.map((entry) { | ||
| final phase = entry.value; | ||
| final color = _phaseColors[entry.key % _phaseColors.length]; | ||
| final fraction = | ||
| (phase.endWeek - phase.startWeek + 1) / program.totalWeeks; | ||
| return Expanded( | ||
| flex: ((fraction * 100).round()).clamp(1, 100), | ||
| child: Container( | ||
| margin: const EdgeInsets.symmetric(horizontal: 1), | ||
| decoration: BoxDecoration( | ||
| color: color, | ||
| borderRadius: BorderRadius.circular(2), | ||
| ), | ||
| ), | ||
| ); | ||
| }).toList(), | ||
| ), | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
Guard against division by zero when totalWeeks is zero.
If program.totalWeeks is 0 (malformed data or edge case), Line 279 will cause a division-by-zero resulting in Infinity or NaN, which could crash or produce undefined UI behavior.
Suggested fix
Widget _buildMiniTimeline() {
+ if (program.totalWeeks <= 0) return const SizedBox.shrink();
return SizedBox(
height: 4,
child: Row(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workout-logger/lib/screens/programs/programs_screen.dart` around lines 271 -
293, The mini-timeline calculation in _buildMiniTimeline uses program.totalWeeks
directly causing division-by-zero if totalWeeks == 0; guard by checking
program.totalWeeks (or compute a safeDenominator = program.totalWeeks > 0 ?
program.totalWeeks : 1) before computing fraction, or skip/return an empty/safe
widget when totalWeeks is 0, then compute fraction = (phase.endWeek -
phase.startWeek + 1) / safeDenominator and continue using that fraction when
building the Expanded flex so no Infinity/NaN values are produced.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/import_program_screen.dart`:
- Around line 89-91: The displayed instruction string in the Text widget inside
import_program_screen.dart contains a double space "Tap to pick" implying a
missing icon; update the Text content used in the child: Text(...) (the literal
string 'Tap to pick a .json file, or paste JSON below. Required: name ·
totalWeeks · phases · weeks.') to include a clear icon or description (e.g., add
an emoji like "📁" or the words "the folder icon") so the message reads
unambiguously (for example "Tap 📁 to pick a .json file, or paste JSON below.
Required: name · totalWeeks · phases · weeks.").
- Around line 364-385: The screen currently reads ProgramManager via
context.read<WorkoutProvider>() inside _import(), coupling the widget to
WorkoutProvider; change the widget to accept a ProgramManager directly (add a
required ProgramManager parameter to ImportProgramScreen and store it on the
State via widget.programManager) or read ProgramManager from its own provider
(context.read<ProgramManager>()) and replace all usages of
context.read<WorkoutProvider>().programManager with the new direct reference;
update the constructor call sites to pass the ProgramManager and adjust
_import() to call widget.programManager.importFromJson(json) (or the locally
read ProgramManager) instead.
- Around line 349-362: The function _requireField currently takes three
positional parameters; change its signature to use named parameters (e.g.,
{_required Map<String, dynamic> map, required String key, required Type type})
and update every call site to pass arguments by name (map: ..., key: ..., type:
...); ensure nullability/required keywords match existing usage and keep the
same runtime checks and thrown FormatException logic inside _requireField
unchanged.
- Around line 15-17: The import of dart:io and direct use of File blocks future
web support; update the import/usages so file reading falls back to the picked
file bytes on web: replace direct File(...) usage in the import logic (the code
that reads the picked file, currently using File and
readAsStringSync/readAsBytes) with a platform-aware branch using kIsWeb (from
foundation) — when kIsWeb use result.files.single.bytes and decode via
utf8.decode (or readAsString from the picked file bytes), otherwise use
File(path). Also either remove unconditional import of dart:io or convert it to
a conditional import so web builds won’t reference dart:io.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1e295b19-5a2a-4f58-b9db-a88ef0caafa1
📒 Files selected for processing (1)
workout-logger/lib/screens/programs/import_program_screen.dart
Comment on lines
+15
to
+17
| import 'dart:convert'; | ||
| import 'dart:io'; | ||
| import 'package:file_picker/file_picker.dart'; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash# Check if project targets web platformecho"=== Checking pubspec.yaml for web configuration ==="
cat pubspec.yaml 2>/dev/null | head -50
echo""echo"=== Checking for existing dart:io usage patterns in screens ==="
rg -l "dart:io" --type dart --glob 'lib/screens/**'Repository: Devasy/Workout-logger
Length of output: 347
🏁 Script executed:
#!/bin/bash# Find pubspec.yaml in the repositoryecho"=== Finding pubspec.yaml ==="
find . -name "pubspec.yaml" -type f 2>/dev/null
echo""echo"=== Repository root structure ==="
ls -la | head -20Repository: Devasy/Workout-logger
Length of output: 1346
🏁 Script executed:
#!/bin/bash# Read pubspec.yaml to check if web is a targetecho"=== Reading pubspec.yaml ==="
cat workout-logger/pubspec.yaml
echo""echo"=== Checking import_program_screen.dart for dart:io usage ==="
head -25 workout-logger/lib/screens/programs/import_program_screen.dartRepository: Devasy/Workout-logger
Length of output: 5468
🏁 Script executed:
#!/bin/bash# Check the actual dart:io usage in the fileecho"=== Checking dart:io usage around line 279 ==="
sed -n '270,290p' workout-logger/lib/screens/programs/import_program_screen.dart
echo""echo"=== Checking analysis_options.yaml ==="
cat workout-logger/analysis_options.yaml 2>/dev/null ||echo"Not found at root"
find workout-logger -name "analysis_options.yaml" -type f -exec cat {} \;Repository: Devasy/Workout-logger
Length of output: 3634
dart:io usage blocks future web support, but web is not currently a target.
The current project targets Android only (web not configured in pubspec.yaml). File from dart:io on line 279 is compatible with mobile platforms. If web support is planned in the future, refactor to use result.files.single.bytes as a fallback or add conditional imports with kIsWeb.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workout-logger/lib/screens/programs/import_program_screen.dart` around lines
15 - 17, The import of dart:io and direct use of File blocks future web support;
update the import/usages so file reading falls back to the picked file bytes on
web: replace direct File(...) usage in the import logic (the code that reads the
picked file, currently using File and readAsStringSync/readAsBytes) with a
platform-aware branch using kIsWeb (from foundation) — when kIsWeb use
result.files.single.bytes and decode via utf8.decode (or readAsString from the
picked file bytes), otherwise use File(path). Also either remove unconditional
import of dart:io or convert it to a conditional import so web builds won’t
reference dart:io.
Comment on lines
+89
to
+91
| child: Text( | ||
| 'Tap to pick a .json file, or paste JSON below. ' | ||
| 'Required: name · totalWeeks · phases · weeks.', |
Contributor
There was a problem hiding this comment.
Instruction text appears incomplete.
The text 'Tap to pick' has a double space where an icon reference seems intended. Consider adding the icon name or emoji for clarity, e.g., 'Tap 📁 to pick a .json file' or 'Tap the folder icon to pick'.
Suggested fix
child: Text(
- 'Tap to pick a .json file, or paste JSON below. '+ 'Tap the folder icon to pick a .json file, or paste JSON below. '
'Required: name · totalWeeks · phases · weeks.',📝 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
| child:Text( | |
| 'Tap to pick a .json file, or paste JSON below. ' | |
| 'Required: name · totalWeeks · phases · weeks.', | |
| child:Text( | |
| 'Tap the folder icon to pick a .json file, or paste JSON below. ' | |
| 'Required: name · totalWeeks · phases · weeks.', |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workout-logger/lib/screens/programs/import_program_screen.dart` around lines
89 - 91, The displayed instruction string in the Text widget inside
import_program_screen.dart contains a double space "Tap to pick" implying a
missing icon; update the Text content used in the child: Text(...) (the literal
string 'Tap to pick a .json file, or paste JSON below. Required: name ·
totalWeeks · phases · weeks.') to include a clear icon or description (e.g., add
an emoji like "📁" or the words "the folder icon") so the message reads
unambiguously (for example "Tap 📁 to pick a .json file, or paste JSON below.
Required: name · totalWeeks · phases · weeks.").
Comment on lines
+349
to
+362
| void _requireField(Map<String, dynamic> map, String key, Type type) { | ||
| if (!map.containsKey(key)) { | ||
| throw FormatException('Missing required field: "$key"'); | ||
| } | ||
| if (type == int && map[key] is! int) { | ||
| throw FormatException('"$key" must be an integer, got ${map[key].runtimeType}'); | ||
| } | ||
| if (type == String && map[key] is! String) { | ||
| throw FormatException('"$key" must be a string, got ${map[key].runtimeType}'); | ||
| } | ||
| if (type == List && map[key] is! List) { | ||
| throw FormatException('"$key" must be an array, got ${map[key].runtimeType}'); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use named parameters for functions with 3+ arguments.
Per coding guidelines, functions with 3 or more arguments should use named parameters for clarity.
Suggested fix
- void _requireField(Map<String, dynamic> map, String key, Type type) {+ void _requireField({+ required Map<String, dynamic> map,+ required String key,+ required Type type,+ }) {
if (!map.containsKey(key)) {
throw FormatException('Missing required field: "$key"');
}Update call sites:
- _requireField(decoded, 'name', String);- _requireField(decoded, 'totalWeeks', int);+ _requireField(map: decoded, key: 'name', type: String);+ _requireField(map: decoded, key: 'totalWeeks', type: int);As per coding guidelines: "Use named parameters for functions with 3 or more arguments for clarity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workout-logger/lib/screens/programs/import_program_screen.dart` around lines
349 - 362, The function _requireField currently takes three positional
parameters; change its signature to use named parameters (e.g., {_required
Map<String, dynamic> map, required String key, required Type type}) and update
every call site to pass arguments by name (map: ..., key: ..., type: ...);
ensure nullability/required keywords match existing usage and keep the same
runtime checks and thrown FormatException logic inside _requireField unchanged.
Comment on lines
+364
to
+385
| Future<void> _import() async { | ||
| if (_parsed == null) return; | ||
| try { | ||
| final provider = context.read<WorkoutProvider>(); | ||
| // Re-encode the validated parsed map to pass through importFromJson | ||
| final json = jsonEncode(_parsed); | ||
| await provider.programManager.importFromJson(json); | ||
| if (mounted) { | ||
| Navigator.pop(context, true); // signal success to caller | ||
| } | ||
| } catch (e) { | ||
| if (!mounted) return; | ||
| ScaffoldMessenger.of(context).showSnackBar( | ||
| SnackBar( | ||
| content: Text('Import failed: ${_shortError(e)}'), | ||
| backgroundColor: AppTheme.error, | ||
| ), | ||
| ); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider injecting ProgramManager directly to reduce coupling.
The screen accesses context.read<WorkoutProvider>().programManager, coupling it to the full WorkoutProvider. Per architectural learnings, components should depend on the smallest scoped manager needed. Consider passing ProgramManager via constructor or a dedicated provider.
Alternative approach
classImportProgramScreenextendsStatefulWidget {
finalProgramManager programManager;
constImportProgramScreen({super.key, requiredthis.programManager});
// ...
}
// In _import():await widget.programManager.importFromJson(json);Or provide ProgramManager separately in the widget tree.
Based on learnings: "Avoid broad context.watch<WorkoutProvider>() in leaf widgets; instead, watch the smallest scoped manager needed to reduce coupling and unnecessary rebuilds."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workout-logger/lib/screens/programs/import_program_screen.dart` around lines
364 - 385, The screen currently reads ProgramManager via
context.read<WorkoutProvider>() inside _import(), coupling the widget to
WorkoutProvider; change the widget to accept a ProgramManager directly (add a
required ProgramManager parameter to ImportProgramScreen and store it on the
State via widget.programManager) or read ProgramManager from its own provider
(context.read<ProgramManager>()) and replace all usages of
context.read<WorkoutProvider>().programManager with the new direct reference;
update the constructor call sites to pass the ProgramManager and adjust
_import() to call widget.programManager.importFromJson(json) (or the locally
read ProgramManager) instead.
- WorkoutFlowScreen: add programDay/programWeek optional params so any day in a program can be started directly from the detail screen - Slot-based rest time: rest seconds are pulled from ProgramExerciseSlot and applied automatically when completing each set - Superset auto-advance: completing a set advances immediately to the next exercise (no rest timer) when both share the same supersetGroupId - Program meta banner: shown above the SET DONE button during program-mode workouts; displays target sets×rep-range, rest duration, tempo, 1RM %, deload indicator, superset label, and slot notes - Pull-up/chin-up assist mode: weight input label changes to 'Assist kg (0=BW)' for pull_ups and chin_ups exercise IDs - ProgramDetailScreen: add 'Start <DayName>' ElevatedButton at the bottom of each expanded day section; navigates to WorkoutFlowScreen with the correct programDay and programWeek context https://claude.ai/code/session_01E15CGTZZDU8rz7PX1nYXA3
Supersets now follow the correct E1→E2 (no rest)→rest→E1→E2 cycle: - Add WorkoutProvider.goToExercise(index) for direct index navigation - Track _supersetReturnIndex: set when completing the last exercise in a superset group if the group still has unfinished sets - _skipRest() now navigates back to the superset group start when _supersetReturnIndex is set, restoring the correct exercise and rest time - Add _supersetGroupStart() to scan backward and find the group's first index - Add _supersetNeedsMoreSets() to check deload-adjusted target sets vs logged Flow: set1(E1) → advance to E2 → set1(E2) → rest → return to E1 → set2(E1) → advance to E2 → set2(E2) → rest → continue to E3 https://claude.ai/code/session_01E15CGTZZDU8rz7PX1nYXA3
Devasy
commented
Mar 25, 2026
OwnerAuthor
@coderabbitai are there any major bugs or concerns that still need to be addressed? |
Contributor
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me inspect the key source files to identify real bugs and concerns. 🏁 Script executed: Length of output: 63703 Let me also check the models and designer screen. 🏁 Script executed: Length of output: 13776 Let me look at a few more critical sections. 🏁 Script executed: Length of output: 3941
🐛 Bug 1: |
| Item | Status |
|---|---|
pull_ups / chin_ups exercise IDs in assist mode | ✅ Matches exercise_database.dart lines 161/171 |
goToExercise bounds check | ✅ Has proper index >= 0 && index < _currentExerciseLogs.length guard |
_rebuildWeeks phase assignment | ✅ Correction loop at lines 785–794 properly overwrites all weeks using actual phase ranges |
ProgramManager.importFromJson UUID reassignment | ✅ New UUID prevents ID collisions with existing programs |
MockStorageService CRUD for TrainingProgram | ✅ Complete and correct |
| Rest timer navigation safety during superset cycle | ✅ Rest timer view has no Next/Previous buttons, so _supersetReturnIndex can't be invalidated by user navigation |
The isImported overwrite in _save() is the most concrete data-integrity bug to fix. Would you like me to open a GitHub issue to track the missing ProgramManager unit tests?
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
workout-logger/lib/services/workout_provider.dart (1)
67-70: 🛠️ Refactor suggestion | 🟠 MajorInject
ProgramManagerinstead of constructing it here.This pulls object-graph wiring back into
WorkoutProvider, which makes focused tests harder and keeps the provider growing new responsibilities. KeepProgramManagercreation inAppInitializerand pass it through the constructor instead.Based on learnings: Use `AppInitializer` in `main.dart` as the composition root for all dependency injection, wiring services via constructor injection into `WorkoutProvider`.♻️ Possible refactor
- late final ProgramManager programManager;+ final ProgramManager programManager; ... - WorkoutProvider(this._storage, {IMLService? mlService})- : _mlService = mlService ?? MLService() {- programManager = ProgramManager(_storage);- }+ WorkoutProvider(+ this._storage, {+ required this.programManager,+ IMLService? mlService,+ }) : _mlService = mlService ?? MLService();🤖 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 67 - 70, The WorkoutProvider currently constructs ProgramManager inside its constructor (see WorkoutProvider(...) and the assignment programManager = ProgramManager(_storage)); change the API to accept a ProgramManager via constructor injection instead: add a ProgramManager parameter to the WorkoutProvider constructor and remove the internal ProgramManager(...) instantiation, and update the DI/wiring in AppInitializer/main.dart to create and pass the ProgramManager into WorkoutProvider; ensure any tests or callers are updated to provide a mock or real ProgramManager as needed.workout-logger/lib/screens/programs/program_detail_screen.dart (1)
455-460:⚠️ Potential issue | 🟠 MajorBuild superset runs without reordering the day.
This map-based grouping still collapses every standalone slot under the same
nullkey and merges repeatedsupersetGroupIds across the whole day. A sequence likeA, [B/C], Drenders asA, D, [B/C], so the detail screen no longer matches the execution order. Build contiguous runs instead of a global map.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workout-logger/lib/screens/programs/program_detail_screen.dart` around lines 455 - 460, The current grouping uses a map (supersetGroups) which collapses all nulls and non-contiguous supersetGroupId occurrences, breaking original order; replace it by building contiguous runs while iterating day.exercises in sequence: create a List<List<ProgramExerciseSlot>> (or similar) called runs, track a currentRunKey and currentRun list, for each slot check slot.supersetGroupId — if null flush any open currentRun and push the slot as its own single-item run; if non-null and equals currentRunKey append to currentRun; if non-null and different, flush previous run and start a new currentRun with that key; after the loop flush any remaining run and use runs instead of supersetGroups in the UI rendering so display order matches execution order.
🤖 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 167-189: Change the three helper functions that accept three
positional parameters—_statChip, _buildSupersetGroup, and _buildExerciseRow—to
use named parameters (mark required where appropriate) so callers are
self-documenting; update all call sites to pass arguments by name (e.g., icon:
..., label: ..., color: ... for _statChip) and adjust any imports/exports if
signatures are public, ensuring types and nullability are preserved and default
values are added only if intended.
- Around line 696-702: The switch in _handleMenuAction currently has
non-terminating cases and the function is marked async despite no awaits; update
_handleMenuAction to remove the async modifier and add a terminating statement
(e.g., break, return, or throw) to each non-empty case — specifically ensure the
'export' case calls _exportProgram() then breaks, and the 'delete' case calls
_confirmDelete() then breaks (or return) so the Dart switch compiles.
In `@workout-logger/lib/screens/workout_flow_screen.dart`:
- Around line 1244-1265: The auto-advance logic uses isSupersetPair to jump to
the next slot solely based on matching supersetGroupId; change it to also
require that the next slot still needs sets before calling
provider.nextExercise() and loading the next slot. In practice, update the
condition around isSupersetPair to check the next slot's remaining-set status
(use or add a helper analogous to _supersetNeedsMoreSets / a per-slot
remaining-sets check) and only call provider.nextExercise(),
_loadLastSessionData(), and update _restSeconds when that check returns true;
otherwise fall through to the existing branch that handles superset completion
and schedules a return via _supersetReturnIndex and _startRestTimer(). Ensure
you reference currentSlot, nextSlot, provider.nextExercise(), _slotForIndex,
_restSeconds, and _supersetNeedsMoreSets (or the new per-slot helper) when
implementing this gate.
- Around line 81-85: Change the functions that take three positional args to use
named parameters for clarity: update the signatures of _supersetNeedsMoreSets
and _programChip to accept named parameters (e.g. {required int startIdx,
required int endIdx, required WorkoutProvider provider}) and mark required where
appropriate, then update all call sites to pass arguments by name (startIdx:
..., endIdx: ..., provider: ...). Also apply the same change to the other helper
referenced around lines 1099-1107, update any documentation/comments, and run
static analysis to ensure no remaining positional calls.
---
Duplicate comments:
In `@workout-logger/lib/screens/programs/program_detail_screen.dart`:
- Around line 455-460: The current grouping uses a map (supersetGroups) which
collapses all nulls and non-contiguous supersetGroupId occurrences, breaking
original order; replace it by building contiguous runs while iterating
day.exercises in sequence: create a List<List<ProgramExerciseSlot>> (or similar)
called runs, track a currentRunKey and currentRun list, for each slot check
slot.supersetGroupId — if null flush any open currentRun and push the slot as
its own single-item run; if non-null and equals currentRunKey append to
currentRun; if non-null and different, flush previous run and start a new
currentRun with that key; after the loop flush any remaining run and use runs
instead of supersetGroups in the UI rendering so display order matches execution
order.
In `@workout-logger/lib/services/workout_provider.dart`:
- Around line 67-70: The WorkoutProvider currently constructs ProgramManager
inside its constructor (see WorkoutProvider(...) and the assignment
programManager = ProgramManager(_storage)); change the API to accept a
ProgramManager via constructor injection instead: add a ProgramManager parameter
to the WorkoutProvider constructor and remove the internal ProgramManager(...)
instantiation, and update the DI/wiring in AppInitializer/main.dart to create
and pass the ProgramManager into WorkoutProvider; ensure any tests or callers
are updated to provide a mock or real ProgramManager as needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b5b68777-942c-48c9-a5de-6ebfdbdadc7f
📒 Files selected for processing (3)
workout-logger/lib/screens/programs/program_detail_screen.dartworkout-logger/lib/screens/workout_flow_screen.dartworkout-logger/lib/services/workout_provider.dart
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.
… dropsets, alongside program design and detail screens.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (4)
workout-logger/lib/services/workout_provider.dart (2)
66-73: 🛠️ Refactor suggestion | 🟠 MajorKeep
ProgramManagerconstruction inAppInitializer.Defaulting to
ProgramManager(_storage)here pulls object-graph construction back intoWorkoutProvider, which makes focused tests and alternate manager implementations harder.Based on learnings: Use
AppInitializerinmain.dartas the composition root for all dependency injection, wiring services via constructor injection intoWorkoutProvider.🤖 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 66 - 73, The WorkoutProvider constructor currently creates ProgramManager itself (ProgramManager(_storage)), pulling composition into the class; change the constructor to stop constructing ProgramManager: accept ProgramManager? programManager (or better, require ProgramManager programManager) and assign it directly (remove programManager ?? ProgramManager(_storage)); then wire ProgramManager creation into your composition root (AppInitializer in main.dart) by constructing ProgramManager with the shared storage and passing that instance into WorkoutProvider when you compose the app; keep the existing mlService defaulting behavior if desired.
40-40:⚠️ Potential issue | 🟠 MajorMove
ProgramManagerinstantiation to the composition root inmain.dart.The constructor fallback at line 73 (
programManager ?? ProgramManager(_storage)) violates the composition root pattern. Per the learnings,AppInitializerinmain.dartshould instantiate and injectProgramManagerdirectly intoWorkoutProvider, not defer creation to the provider's constructor. This ensures all dependencies are wired upfront and prevents accidental multiple instantiations.While
programs_screen.dartdirectly listens toprogramManager(line 24) and will correctly receive notifications, this tight coupling should be resolved by providingProgramManageras its ownChangeNotifierProviderin the provider tree, allowing other screens and features to depend on it explicitly rather than throughWorkoutProvider.🤖 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` at line 40, Remove the constructor fallback that creates ProgramManager inside WorkoutProvider and instead require ProgramManager to be injected from the composition root: delete the `programManager ?? ProgramManager(_storage)` instantiation in the WorkoutProvider constructor and make ProgramManager a non-null constructor parameter; in main.dart/AppInitializer instantiate a single ProgramManager and add it to the provider tree as its own ChangeNotifierProvider, then pass that instance into WorkoutProvider when registering it; update consumers (e.g., programs_screen.dart) to read/listen to ProgramManager from its dedicated ChangeNotifierProvider rather than reaching through WorkoutProvider so other features can depend on ProgramManager directly.workout-logger/lib/screens/programs/program_designer_screen.dart (2)
928-932:⚠️ Potential issue | 🔴 Critical
_NumberSteppercurrently passesnumintoValueChanged<int>.Line 931 and Line 950 use
clamp(), whose static return type isnum, so these callbacks do not type-check.Possible fix
IconButton( icon: const Icon(Icons.remove, size: 18), onPressed: value > min - ? () => onChanged((value - step).clamp(min, max))+ ? () {+ final next = value - step;+ onChanged(next < min ? min : next);+ } : null, padding: EdgeInsets.zero, constraints: const BoxConstraints(minWidth: 32, minHeight: 32), ), @@ IconButton( icon: const Icon(Icons.add, size: 18), onPressed: value < max - ? () => onChanged((value + step).clamp(min, max))+ ? () {+ final next = value + step;+ onChanged(next > max ? max : next);+ } : null, padding: EdgeInsets.zero, constraints: const BoxConstraints(minWidth: 32, minHeight: 32), ),In Dart, what is the static return type of `int.clamp(num lowerLimit, num upperLimit)`, and can that result be passed directly to a `ValueChanged<int>` callback without an explicit conversion?Also applies to: 947-951
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workout-logger/lib/screens/programs/program_designer_screen.dart` around lines 928 - 932, The callbacks in _NumberStepper pass the result of .clamp(...) (which has static type num) to a ValueChanged<int> (onChanged), causing a type mismatch; update the decrement and increment closures (the IconButton onPressed handlers around the onChanged calls) and any other clamp usages in _NumberStepper to convert the clamped num to an int before calling onChanged (for example, call .toInt() on the clamp result) so the value passed to onChanged is an int.
713-724:⚠️ Potential issue | 🟠 MajorPreserve
supersetGroupIdwhen editing a slot.Rebuilding
ProgramExerciseSlotwithout that field strips imported/existing supersets the first time the slot is edited.Possible fix
final slot = ProgramExerciseSlot( exerciseId: selectedExerciseId!, sets: sets, minReps: minReps, maxReps: maxReps, restSeconds: restSec, tempo: tempoCtrl.text.isEmpty ? null : tempoCtrl.text, weightPercentage: double.tryParse(weightPctCtrl.text), notes: notesCtrl.text.isEmpty ? null : notesCtrl.text, + supersetGroupId: existing?.supersetGroupId, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workout-logger/lib/screens/programs/program_designer_screen.dart` around lines 713 - 724, The new ProgramExerciseSlot created in the onPressed handler omits supersetGroupId, which drops existing/imported supersets when editing; when rebuilding the slot (in the onPressed callback) include supersetGroupId: existingSlot.supersetGroupId (or whatever variable holds the slot being edited) in the ProgramExerciseSlot constructor so the original supersetGroupId is preserved during edits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/skills/flutter-expert/SKILL.md:
- Line 287: Add a single trailing newline at the end of the file so the final
line ("Always prioritize native performance, beautiful UI, and consistent
experience while building Flutter applications that delight users across all
platforms.") is terminated with a newline character; simply open the SKILL.md
file, move to the end, and insert one newline so the file ends with exactly one
trailing newline.
- Around line 133-143: Insert a blank line immediately before the fenced JSON
code block that begins with ```json under the "Flutter context query:" heading
so the code block is surrounded by a blank line as required by markdownlint;
locate the "Flutter context query:" paragraph and add one empty line above the
```json fence (and ensure there's a blank line after the closing ``` if not
already present) to satisfy the lint rule.
- Around line 196-209: The fenced JSON code block under the "Progress tracking:"
heading in SKILL.md is not surrounded by blank lines; update the markdown so
there is an empty line before the ```json fence and an empty line after the
closing ``` to satisfy markdownlint (surround the JSON code block with blank
lines).
- Around line 17-26: The checklist items contain awkward phrasing; update the
listed strings to clear, parallel, actionable phrases by replacing "Null safety
enforced properly maintained" with "Null safety enforced", "Performance 60 FPS
consistently delivered" with "Consistent 60 FPS performance", "Bundle size
optimized thoroughly completed" with "Bundle size optimized", "Platform parity
maintained properly" with "Platform parity maintained", and "Code quality
excellent achieved" with "High code quality"; ensure the items follow the same
grammatical pattern as the other entries (e.g., adjective + noun or verb phrase)
and keep the header "Flutter expert checklist:" and other items unchanged.
In @.github/workflows/test.yml:
- Around line 20-25: Replace the mutable tag used for the GitHub Action in the
workflow (the uses entry "subosito/flutter-action@v2") with an immutable full
commit SHA; update the uses value to the action's commit SHA (e.g., replace
"subosito/flutter-action@v2" with "subosito/flutter-action@<full-commit-sha>")
and keep the original tag as an inline comment for readability (e.g., "# v2") so
the Set up Flutter step continues to reference the same code immutably.
In `@workout-logger/lib/screens/programs/program_designer_screen.dart`:
- Around line 144-149: When totalWeeks is changed or before persisting the
program, validate all phase range definitions to ensure each phase's start/end
are within the new _totalWeeks, that ranges do not overlap, and that every week
maps to at most one phase (since ProgramWeek stores a single phaseId and
_rebuildWeeks() stops at first match). Update the onChanged handler for
_totalWeeks and the save/submit flow to run a validator that (a) rejects or
adjusts shrink operations that would truncate a phase, (b) detects overlapping
ranges and surfaces an error message to the user, and (c) prevents saving until
ranges are fixed; reference the _rebuildWeeks(), _totalWeeks, ProgramWeek, and
the phase range data structures when locating where to insert this check. Ensure
the validator returns clear user-facing errors so overlaps or out-of-bounds
phases can be corrected before persistence.
In `@workout-logger/lib/screens/workout_flow_screen.dart`:
- Around line 1328-1332: _adjustRestTime currently assigns results of
num-returning clamp() calls to int fields (_remainingSeconds and _restSeconds),
causing type errors; change the assignments in the _adjustRestTime method to
convert the clamp() results to int (for example by calling toInt() on the
clamp() result or casting to int) so the values assigned to _remainingSeconds
and _restSeconds are ints.
---
Duplicate comments:
In `@workout-logger/lib/screens/programs/program_designer_screen.dart`:
- Around line 928-932: The callbacks in _NumberStepper pass the result of
.clamp(...) (which has static type num) to a ValueChanged<int> (onChanged),
causing a type mismatch; update the decrement and increment closures (the
IconButton onPressed handlers around the onChanged calls) and any other clamp
usages in _NumberStepper to convert the clamped num to an int before calling
onChanged (for example, call .toInt() on the clamp result) so the value passed
to onChanged is an int.
- Around line 713-724: The new ProgramExerciseSlot created in the onPressed
handler omits supersetGroupId, which drops existing/imported supersets when
editing; when rebuilding the slot (in the onPressed callback) include
supersetGroupId: existingSlot.supersetGroupId (or whatever variable holds the
slot being edited) in the ProgramExerciseSlot constructor so the original
supersetGroupId is preserved during edits.
In `@workout-logger/lib/services/workout_provider.dart`:
- Around line 66-73: The WorkoutProvider constructor currently creates
ProgramManager itself (ProgramManager(_storage)), pulling composition into the
class; change the constructor to stop constructing ProgramManager: accept
ProgramManager? programManager (or better, require ProgramManager
programManager) and assign it directly (remove programManager ??
ProgramManager(_storage)); then wire ProgramManager creation into your
composition root (AppInitializer in main.dart) by constructing ProgramManager
with the shared storage and passing that instance into WorkoutProvider when you
compose the app; keep the existing mlService defaulting behavior if desired.
- Line 40: Remove the constructor fallback that creates ProgramManager inside
WorkoutProvider and instead require ProgramManager to be injected from the
composition root: delete the `programManager ?? ProgramManager(_storage)`
instantiation in the WorkoutProvider constructor and make ProgramManager a
non-null constructor parameter; in main.dart/AppInitializer instantiate a single
ProgramManager and add it to the provider tree as its own
ChangeNotifierProvider, then pass that instance into WorkoutProvider when
registering it; update consumers (e.g., programs_screen.dart) to read/listen to
ProgramManager from its dedicated ChangeNotifierProvider rather than reaching
through WorkoutProvider so other features can depend on ProgramManager directly.
🪄 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: fe8d948d-3d2d-450e-9312-65b5437e628f
📒 Files selected for processing (7)
.github/skills/flutter-expert/SKILL.md.github/workflows/test.ymlworkout-logger/lib/screens/programs/program_designer_screen.dartworkout-logger/lib/screens/programs/program_detail_screen.dartworkout-logger/lib/screens/workout_flow_screen.dartworkout-logger/lib/services/workout_provider.dartworkout-logger/test/program_manager_test.dart
Comment on lines
+17
to
+26
| Flutter expert checklist: | ||
| - Flutter 3+ features utilized effectively | ||
| - Null safety enforced properly maintained | ||
| - Widget tests > 80% coverage achieved | ||
| - Performance 60 FPS consistently delivered | ||
| - Bundle size optimized thoroughly completed | ||
| - Platform parity maintained properly | ||
| - Accessibility support implemented correctly | ||
| - Code quality excellent achieved | ||
Contributor
There was a problem hiding this comment.
Fix awkward phrasing in checklist items.
Several checklist items have grammatically awkward constructions that mix adjectives and past participles incorrectly:
- Line 19: "Null safety enforced properly maintained"
- Line 21: "Performance 60 FPS consistently delivered"
- Line 22: "Bundle size optimized thoroughly completed"
- Line 23: "Platform parity maintained properly"
- Line 25: "Code quality excellent achieved"
✍️ Proposed fix for clearer phrasing
Flutter expert checklist:
- Flutter 3+ features utilized effectively
-- Null safety enforced properly maintained+- Null safety properly enforced
- Widget tests > 80% coverage achieved
-- Performance 60 FPS consistently delivered+- Performance consistently at 60 FPS-- Bundle size optimized thoroughly completed+- Bundle size thoroughly optimized-- Platform parity maintained properly+- Platform parity properly maintained
- Accessibility support implemented correctly
-- Code quality excellent achieved+- Excellent code quality achieved📝 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
| Flutter expert checklist: | |
| - Flutter 3+ features utilized effectively | |
| - Null safety enforced properly maintained | |
| - Widget tests > 80% coverage achieved | |
| - Performance 60 FPS consistently delivered | |
| - Bundle size optimized thoroughly completed | |
| - Platform parity maintained properly | |
| - Accessibility support implemented correctly | |
| - Code quality excellent achieved | |
| Flutter expert checklist: | |
| - Flutter 3+ features utilized effectively | |
| - Null safety properly enforced | |
| - Widget tests > 80% coverage achieved | |
| - Performance consistently at 60 FPS | |
| - Bundle size thoroughly optimized | |
| - Platform parity properly maintained | |
| - Accessibility support implemented correctly | |
| - Excellent code quality achieved | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/skills/flutter-expert/SKILL.md around lines 17 - 26, The checklist
items contain awkward phrasing; update the listed strings to clear, parallel,
actionable phrases by replacing "Null safety enforced properly maintained" with
"Null safety enforced", "Performance 60 FPS consistently delivered" with
"Consistent 60 FPS performance", "Bundle size optimized thoroughly completed"
with "Bundle size optimized", "Platform parity maintained properly" with
"Platform parity maintained", and "Code quality excellent achieved" with "High
code quality"; ensure the items follow the same grammatical pattern as the other
entries (e.g., adjective + noun or verb phrase) and keep the header "Flutter
expert checklist:" and other items unchanged.
Comment on lines
+133
to
+143
| Flutter context query: | ||
| ```json | ||
| { | ||
| "requesting_agent": "flutter-expert", | ||
| "request_type": "get_flutter_context", | ||
| "payload": { | ||
| "query": "Flutter context needed: target platforms, app type, state management preference, native features required, and deployment strategy." | ||
| } | ||
| } | ||
| ``` | ||
Contributor
There was a problem hiding this comment.
Add blank line before code block.
Static analysis (markdownlint) reports that fenced code blocks should be surrounded by blank lines.
📝 Proposed fix
Flutter context query:
+
```json
{
"requesting_agent": "flutter-expert",🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 134-134: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/skills/flutter-expert/SKILL.md around lines 133 - 143, Insert a
blank line immediately before the fenced JSON code block that begins with
```json under the "Flutter context query:" heading so the code block is
surrounded by a blank line as required by markdownlint; locate the "Flutter
context query:" paragraph and add one empty line above the ```json fence (and
ensure there's a blank line after the closing ``` if not already present) to
satisfy the lint rule.
Uh oh!
There was an error while loading. Please reload this page.
| - Partner with backend-developer on API integration | ||
| - Coordinate with ios-developer on iOS specifics | ||
| Always prioritize native performance, beautiful UI, and consistent experience while building Flutter applications that delight users across all platforms. No newline at end of file |
Contributor
There was a problem hiding this comment.
Add trailing newline at end of file.
Static analysis (markdownlint) reports that files should end with a single newline character.
📝 Proposed fix
Always prioritize native performance, beautiful UI, and consistent experience while building Flutter applications that delight users across all platforms.
+📝 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
| Always prioritize native performance, beautiful UI, and consistent experience while building Flutter applications that delight users across all platforms. | |
| Always prioritize native performance, beautiful UI, and consistent experience while building Flutter applications that delight users across all platforms. | |
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 287-287: Files should end with a single newline character
(MD047, single-trailing-newline)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/skills/flutter-expert/SKILL.md at line 287, Add a single trailing
newline at the end of the file so the final line ("Always prioritize native
performance, beautiful UI, and consistent experience while building Flutter
applications that delight users across all platforms.") is terminated with a
newline character; simply open the SKILL.md file, move to the end, and insert
one newline so the file ends with exactly one trailing newline.
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.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/test.yml:
- Around line 35-45: The "Analyze" step currently uses tail -n 1 on
analyze_output.txt which is fragile; update the extraction to reliably parse
flutter analyze output by grepping for a known summary pattern from
analyze_output.txt instead of taking the last line: run flutter analyze
--no-fatal-infos --no-fatal-warnings | tee analyze_output.txt as before, then
replace the SUMMARY=$(tail -n 1 analyze_output.txt) assignment with a grep
command that matches the analyzer summary line (e.g., match strings like "No
issues found", "Analyzed", or the numeric issues pattern) and fallback to a safe
default if no match is found, and continue echoing that SUMMARY into
$GITHUB_STEP_SUMMARY so the "Analyze" job produces a stable summary even if
output formatting changes.
🪄 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: 2f7fa6e8-a5f5-43bc-bcb7-1fa9b6fd1cc0
📒 Files selected for processing (2)
.github/workflows/test.ymlworkout-logger/pubspec.yaml
Comment on lines
+35
to
+45
| - name: Analyze | ||
| working-directory: ./workout-logger | ||
| run: | | ||
| # Only fail on errors, ignore warnings and info messages | ||
| flutter analyze --no-fatal-infos --no-fatal-warnings | tee analyze_output.txt | ||
| # Extract and display the summary of issues | ||
| SUMMARY=$(tail -n 1 analyze_output.txt) | ||
| echo "Analysis Summary: $SUMMARY" | ||
| echo "### Analysis Summary" >> $GITHUB_STEP_SUMMARY | ||
| echo "$SUMMARY" >> $GITHUB_STEP_SUMMARY |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Summary extraction via tail -n 1 is fragile.
The analysis summary extraction assumes the last line always contains the meaningful summary. This can break if:
flutter analyzeoutput format changes- Output contains trailing blank lines
- Multi-line summaries are introduced
Consider using grep to match a known pattern instead:
♻️ Suggested improvement
- SUMMARY=$(tail -n 1 analyze_output.txt)+ # Extract the line containing issue counts (e.g., "No issues found!" or "X issues found")+ SUMMARY=$(grep -E '(issues? found|No issues)' analyze_output.txt | tail -n 1)+ SUMMARY=${SUMMARY:-"See analyze_output.txt for details"}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/test.yml around lines 35 - 45, The "Analyze" step
currently uses tail -n 1 on analyze_output.txt which is fragile; update the
extraction to reliably parse flutter analyze output by grepping for a known
summary pattern from analyze_output.txt instead of taking the last line: run
flutter analyze --no-fatal-infos --no-fatal-warnings | tee analyze_output.txt as
before, then replace the SUMMARY=$(tail -n 1 analyze_output.txt) assignment with
a grep command that matches the analyzer summary line (e.g., match strings like
"No issues found", "Analyzed", or the numeric issues pattern) and fallback to a
safe default if no match is found, and continue echoing that SUMMARY into
$GITHUB_STEP_SUMMARY so the "Analyze" job produces a stable summary even if
output formatting changes.
…xerciseScreen, along with project configuration in pubspec.yaml.
…ercises, and workout flow, supported by new screens and tests.
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
2 participants
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.
Documents project structure, architecture patterns (SOLID), data models,
development commands, testing conventions, CI/CD pipeline, theme system,
and key conventions for AI assistants working on the codebase.
https://claude.ai/code/session_01NytaxeQoLadhKcESsKqjmT
Summary by CodeRabbit
New Features
Chores
Tests
CI