feat: Add clone routine functionality - #46
Conversation
- Add 'Clone Routine' option in the long-press options menu. - Show an input dialog with the original routine's name as a hint. - Create a copy of the routine using the entered name (or the original if empty). - Include widget tests in `routines_screen_test.dart` to verify cloning. Co-authored-by: Devasy23 <110348311+Devasy23@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
WalkthroughThis PR adds a "Clone Routine" feature to the routines screen. Users can long-press a routine to open a bottom sheet, select "Clone Routine", enter a new name (or accept the default), and create a duplicate routine with the same exercises. Comprehensive tests verify both custom-name and default-name cloning scenarios. ChangesRoutine Cloning Feature
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #46 +/- ##
==========================================
+ Coverage 29.13% 33.40% +4.27%
==========================================
Files 33 36 +3
Lines 5999 6208 +209
==========================================
+ Hits 1748 2074 +326
+ Misses 4251 4134 -117 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@workout-logger/lib/screens/routines_screen.dart`:
- Around line 308-315: The onPressed handler for the FilledButton calls the
async provider.createRoutine(...) without awaiting it, then immediately calls
Navigator.pop(context), causing the dialog to close before the routine is
saved/added (and swallowing storage errors); change the handler to await
provider.createRoutine(newName, routine.exerciseIds) and only call
Navigator.pop(context) after that await, wrapping the await in try/catch to
surface or log any errors (mirror the pattern used in _saveRoutine) so the UI
only closes after a successful save and errors are handled.
- Around line 287-289: _showCloneDialog currently allocates a
TextEditingController but never disposes it, leaking resources each time the
dialog is opened; fix by creating the TextEditingController before calling
showDialog and then disposing it in the Future completion handler (e.g., in the
.then(...) or await finally) so it is always disposed whether the dialog is
dismissed or accepted; locate _showCloneDialog and ensure the controller is
disposed after the dialog Future completes rather than relying on widget
disposal (since _RoutineCard is StatelessWidget).
In `@workout-logger/test/screens/routines_screen_test.dart`:
- Around line 12-25: Add a tearDown that calls dispose() on the WorkoutProvider
instance created in setUp so the ChangeNotifier is properly cleaned up after
each test; locate the setUp block that constructs WorkoutProvider (and uses
ChangeNotifierProvider.value in tests) and add a matching tearDown that calls
provider.dispose() (or provider.close() if applicable) to prevent listener leaks
and leftover state between tests.
🪄 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: 0a561695-4060-4628-90bd-d1cc44ef7cf3
📒 Files selected for processing (2)
workout-logger/lib/screens/routines_screen.dartworkout-logger/test/screens/routines_screen_test.dart
| void _showCloneDialog(BuildContext context) { | ||
| final TextEditingController nameController = TextEditingController(); | ||
|
|
There was a problem hiding this comment.
TextEditingController is never disposed — resource leak
The Flutter SDK requires calling dispose() on a TextEditingController when it is no longer needed, to ensure any resources used by the object are discarded. Each time a user opens the clone dialog, a new controller is allocated but never freed. Since _RoutineCard is a StatelessWidget, attach disposal to the dialog's returned Future:
🛠️ Proposed fix
void _showCloneDialog(BuildContext context) {
final TextEditingController nameController = TextEditingController();
- showDialog(
+ showDialog<void>(
context: context,
builder: (context) => AlertDialog(
...
),
- );
+ ).then((_) => nameController.dispose());
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@workout-logger/lib/screens/routines_screen.dart` around lines 287 - 289,
_showCloneDialog currently allocates a TextEditingController but never disposes
it, leaking resources each time the dialog is opened; fix by creating the
TextEditingController before calling showDialog and then disposing it in the
Future completion handler (e.g., in the .then(...) or await finally) so it is
always disposed whether the dialog is dismissed or accepted; locate
_showCloneDialog and ensure the controller is disposed after the dialog Future
completes rather than relying on widget disposal (since _RoutineCard is
StatelessWidget).
| FilledButton( | ||
| onPressed: () { | ||
| final newName = nameController.text.trim().isEmpty | ||
| ? routine.name | ||
| : nameController.text.trim(); | ||
| provider.createRoutine(newName, routine.exerciseIds); | ||
| Navigator.pop(context); | ||
| }, |
There was a problem hiding this comment.
createRoutine must be awaited — dialog may close before the clone appears in the list
provider.createRoutine is async; internally it does await _storage.saveRoutine(routine) before calling _routines.add(routine) and notifyListeners(). Because the call is fire-and-forget here, Navigator.pop fires before the routine is actually inserted and the widget tree rebuilds — producing a brief but visible race where the dialog closes but the cloned routine is absent from the list. Storage errors are also silently swallowed. The analogous _saveRoutine method (line 707) already awaits the same call correctly.
🐛 Proposed fix
- onPressed: () {
+ onPressed: () async {
final newName = nameController.text.trim().isEmpty
? routine.name
: nameController.text.trim();
- provider.createRoutine(newName, routine.exerciseIds);
- Navigator.pop(context);
+ await provider.createRoutine(newName, routine.exerciseIds);
+ if (context.mounted) Navigator.pop(context);
},📝 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.
| FilledButton( | |
| onPressed: () { | |
| final newName = nameController.text.trim().isEmpty | |
| ? routine.name | |
| : nameController.text.trim(); | |
| provider.createRoutine(newName, routine.exerciseIds); | |
| Navigator.pop(context); | |
| }, | |
| FilledButton( | |
| onPressed: () async { | |
| final newName = nameController.text.trim().isEmpty | |
| ? routine.name | |
| : nameController.text.trim(); | |
| await provider.createRoutine(newName, routine.exerciseIds); | |
| if (context.mounted) Navigator.pop(context); | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@workout-logger/lib/screens/routines_screen.dart` around lines 308 - 315, The
onPressed handler for the FilledButton calls the async
provider.createRoutine(...) without awaiting it, then immediately calls
Navigator.pop(context), causing the dialog to close before the routine is
saved/added (and swallowing storage errors); change the handler to await
provider.createRoutine(newName, routine.exerciseIds) and only call
Navigator.pop(context) after that await, wrapping the await in try/catch to
surface or log any errors (mirror the pattern used in _saveRoutine) so the UI
only closes after a successful save and errors are handled.
| late MockStorageService mockStorage; | ||
| late WorkoutProvider provider; | ||
|
|
||
| setUp(() async { | ||
| mockStorage = MockStorageService(); | ||
| provider = WorkoutProvider( | ||
| mockStorage, | ||
| programManager: ProgramManager(mockStorage), | ||
| ); | ||
| await provider.init(); | ||
|
|
||
| // Setup a pre-existing routine | ||
| await provider.createRoutine('Leg Day', ['squat', 'leg_press']); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Add a tearDown to dispose WorkoutProvider
ChangeNotifierProvider.value does not take ownership of the provided instance — it never calls dispose(). Without a tearDown, WorkoutProvider (a ChangeNotifier) is never disposed after each test, which can trigger debug-mode warnings about listener leaks and leave residual state if the framework defers cleanup.
♻️ Proposed fix
setUp(() async {
mockStorage = MockStorageService();
provider = WorkoutProvider(
mockStorage,
programManager: ProgramManager(mockStorage),
);
await provider.init();
// Setup a pre-existing routine
await provider.createRoutine('Leg Day', ['squat', 'leg_press']);
});
+
+ tearDown(() {
+ provider.dispose();
+ });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@workout-logger/test/screens/routines_screen_test.dart` around lines 12 - 25,
Add a tearDown that calls dispose() on the WorkoutProvider instance created in
setUp so the ChangeNotifier is properly cleaned up after each test; locate the
setUp block that constructs WorkoutProvider (and uses
ChangeNotifierProvider.value in tests) and add a matching tearDown that calls
provider.dispose() (or provider.close() if applicable) to prevent listener leaks
and leftover state between tests.
1 participant
This PR introduces a "Clone Routine" feature to the Routines screen. Users can long-press a routine and select "Clone Routine". This prompts the user with an alert dialog asking for a new routine name, pre-filled with the original routine's name as a hint. If the user submits an empty name, it defaults to the original name. The routine is cloned with the exact same exercises and added to the routine list. Appropriate widget tests have been added to verify this new functionality.
PR created automatically by Jules for task 13229627913334241519 started by @Devasy23
Summary by CodeRabbit
Release Notes