Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 44 additions & 0 deletions workout-logger/lib/screens/routines_screen.dart
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,14 @@ class _RoutineCard extends StatelessWidget {
);
},
),
ListTile(
leading: const Icon(Icons.copy),
title: const Text('Clone Routine'),
onTap: () {
Navigator.pop(context);
_showCloneDialog(context);
},
),
ListTile(
leading: const Icon(Icons.delete, color: AppTheme.error),
title: const Text(
Expand All @@ -276,6 +284,42 @@ class _RoutineCard extends StatelessWidget {
);
}

void _showCloneDialog(BuildContext context) {
final TextEditingController nameController = TextEditingController();

Comment on lines +287 to +289

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

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).

showDialog(
context: context,
builder: (context) => AlertDialog(
title: const Text('Clone Routine'),
content: TextField(
controller: nameController,
autofocus: true,
decoration: InputDecoration(
hintText: routine.name,
labelText: 'New Routine Name',
),
textCapitalization: TextCapitalization.words,
),
actions: [
TextButton(
onPressed: () => Navigator.pop(context),
child: const Text('Cancel'),
),
FilledButton(
onPressed: () {
final newName = nameController.text.trim().isEmpty
? routine.name
: nameController.text.trim();
provider.createRoutine(newName, routine.exerciseIds);
Navigator.pop(context);
},
Comment on lines +308 to +315

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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.

Suggested change
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.

child: const Text('Clone'),
),
],
),
);
}

void _confirmDelete(BuildContext context) {
showDialog(
context: context,
Expand Down
109 changes: 109 additions & 0 deletions workout-logger/test/screens/routines_screen_test.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
import 'package:flutter/material.dart';
import 'package:flutter_test/flutter_test.dart';
import 'package:provider/provider.dart';
import 'package:repforge/models/models.dart';
import 'package:repforge/screens/routines_screen.dart';
import 'package:repforge/services/managers/program_manager.dart';
import 'package:repforge/services/workout_provider.dart';
import '../test_utils/mock_storage_service.dart';

void main() {
group('RoutinesScreen clone routine tests', () {
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']);
});
Comment on lines +12 to +25

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.


testWidgets('shows clone option and clones routine successfully', (WidgetTester tester) async {
await tester.pumpWidget(
MaterialApp(
home: ChangeNotifierProvider<WorkoutProvider>.value(
value: provider,
child: const RoutinesScreen(),
),
),
);

await tester.pumpAndSettle();

// Find the routine card
final routineCard = find.text('Leg Day');
expect(routineCard, findsOneWidget);

// Long press to open options
await tester.longPress(routineCard);
await tester.pumpAndSettle();

// Find and tap the clone option
final cloneOption = find.text('Clone Routine');
expect(cloneOption, findsOneWidget);
await tester.tap(cloneOption);
await tester.pumpAndSettle();

// Verify clone dialog appears
final dialogTitle = find.text('Clone Routine');
expect(dialogTitle, findsOneWidget);

// Provide a new name
final textField = find.byType(TextField);
expect(textField, findsOneWidget);
await tester.enterText(textField, 'Leg Day Copy');
await tester.pumpAndSettle();

// Tap Clone button
final cloneButton = find.widgetWithText(FilledButton, 'Clone');
expect(cloneButton, findsOneWidget);
await tester.tap(cloneButton);
await tester.pumpAndSettle();

// Verify dialog is closed and new routine is in the provider
expect(find.text('Clone Routine'), findsNothing);
expect(provider.routines.length, 2);
expect(provider.routines[1].name, 'Leg Day Copy');
expect(provider.routines[1].exerciseIds, ['squat', 'leg_press']);

// Verify new routine is shown in UI
expect(find.text('Leg Day Copy'), findsOneWidget);
});

testWidgets('clones with original name if text field is empty', (WidgetTester tester) async {
await tester.pumpWidget(
MaterialApp(
home: ChangeNotifierProvider<WorkoutProvider>.value(
value: provider,
child: const RoutinesScreen(),
),
),
);

await tester.pumpAndSettle();

// Long press to open options
await tester.longPress(find.text('Leg Day'));
await tester.pumpAndSettle();

// Tap clone
await tester.tap(find.text('Clone Routine'));
await tester.pumpAndSettle();

// Do NOT enter text, just tap Clone button
await tester.tap(find.widgetWithText(FilledButton, 'Clone'));
await tester.pumpAndSettle();

// Verify provider has the cloned routine with the original name
expect(provider.routines.length, 2);
expect(provider.routines[1].name, 'Leg Day');
expect(provider.routines[1].exerciseIds, ['squat', 'leg_press']);
});
});
}
Loading