From 6d3e39e588945a093348ef8f8a9bc0eef1d30a28 Mon Sep 17 00:00:00 2001 From: ghost <49853598+JSONbored@users.noreply.github.com> Date: Mon, 29 Jun 2026 22:06:03 -0700 Subject: [PATCH] fix(mobile): dispose API-key dialog controller on any dismissal (#2399) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(mobile): own the API-key dialog controller in its own State _showApiKeyDialog created a TextEditingController in the parent State and only disposed it in the Cancel/Sign-In handlers, so it leaked whenever the dialog was dismissed via a barrier tap or the system back button. Disposing it right after `await showDialog` (an earlier attempt) instead risks disposing the controller while the dialog's TextField is still mounted during the route's exit transition ("TextEditingController was used after being disposed"). Extract the dialog into a small StatefulWidget (_ApiKeyLoginDialog) that owns the controller and disposes it in State.dispose(), tying the controller's lifecycle to the dialog's widget tree. The dialog pops true/false/null and the screen shows the error snackbar on a failed attempt. flutter analyze: no new issues; flutter test: all green. * test(mobile): cover ApiKeyLoginDialog controller disposal Per review: add a widget test for the lifecycle contract that is the core of this change. Exposes the dialog as `ApiKeyLoginDialog` (@visibleForTesting) with an injectable controller, and asserts the controller is disposed on all three dismissal paths — Cancel button, barrier tap, and system back — by checking that using the controller afterward throws (a disposed ChangeNotifier throws on reuse). 119 tests pass; flutter analyze: no new issues. * test(mobile): derive the barrier-tap point from dialog geometry Per review: replace the hard-coded Offset(10, 10) in the barrier-dismissal test. Tapping the ModalBarrier widget directly hits the dialog that occludes its centre, so instead tap halfway between the screen corner and the dialog's top-left — a point derived from the dialog's real geometry, resilient to layout changes, and always on the dismissible barrier. * fix(mobile): keyboard submit + disable Sign In when API key is empty - Add onSubmitted to the API key TextField so the keyboard Done/Enter key submits the form (no need to tap the button). - Wire a controller listener to rebuild the dialog state and disable the Sign In ElevatedButton while the field is blank, giving clear feedback before any network call is made. --- mobile/lib/screens/login_screen.dart | 216 ++++++++++-------- .../widgets/api_key_login_dialog_test.dart | 75 ++++++ 2 files changed, 194 insertions(+), 97 deletions(-) create mode 100644 mobile/test/widgets/api_key_login_dialog_test.dart diff --git a/mobile/lib/screens/login_screen.dart b/mobile/lib/screens/login_screen.dart index 32841ae76..61914dfcc 100644 --- a/mobile/lib/screens/login_screen.dart +++ b/mobile/lib/screens/login_screen.dart @@ -88,105 +88,29 @@ class _LoginScreenState extends State { } } - void _showApiKeyDialog() { - final apiKeyController = TextEditingController(); - final outerContext = context; - bool isLoading = false; - - showDialog( + Future _showApiKeyDialog() async { + // The dialog owns its TextEditingController and disposes it in its own + // State.dispose(), so the controller's lifecycle is tied to the dialog's + // widget tree: no leak on barrier/back dismissal, and it is never disposed + // out from under a still-mounted TextField during the exit transition. + final result = await showDialog( context: context, - builder: (dialogContext) { - final dl = AppLocalizations.of(dialogContext); - return StatefulBuilder( - builder: (_, setDialogState) { - return AlertDialog( - title: Text(dl.loginApiKeyDialogTitle), - content: Column( - mainAxisSize: MainAxisSize.min, - children: [ - Text( - dl.loginApiKeyDialogBody, - style: - Theme.of(outerContext).textTheme.bodyMedium?.copyWith( - color: Theme.of(outerContext) - .colorScheme - .onSurfaceVariant, - ), - ), - const SizedBox(height: 16), - TextField( - controller: apiKeyController, - decoration: InputDecoration( - labelText: dl.loginApiKeyLabel, - prefixIcon: const Icon(Icons.vpn_key_outlined), - ), - obscureText: true, - maxLines: 1, - enabled: !isLoading, - ), - ], - ), - actions: [ - TextButton( - onPressed: isLoading - ? null - : () { - apiKeyController.dispose(); - Navigator.of(dialogContext).pop(); - }, - child: Text(dl.commonCancel), - ), - ElevatedButton( - onPressed: isLoading - ? null - : () async { - final apiKey = apiKeyController.text.trim(); - if (apiKey.isEmpty) return; - - setDialogState(() { - isLoading = true; - }); - - final authProvider = Provider.of( - outerContext, - listen: false, - ); - final success = await authProvider.loginWithApiKey( - apiKey: apiKey, - ); - - if (!dialogContext.mounted) return; - - final errorMsg = authProvider.errorMessage; - apiKeyController.dispose(); - Navigator.of(dialogContext).pop(); - - if (!success && mounted) { - ScaffoldMessenger.of(outerContext).showSnackBar( - SnackBar( - content: Text( - errorMsg ?? dl.loginApiKeyInvalid, - ), - backgroundColor: - Theme.of(outerContext).colorScheme.error, - ), - ); - } - }, - child: isLoading - ? const SizedBox( - height: 20, - width: 20, - child: CircularProgressIndicator(strokeWidth: 2), - ) - : Text(dl.loginApiKeySignIn), - ), - ], - ); - }, - ); - }, + builder: (_) => const ApiKeyLoginDialog(), ); + + // result: true = signed in, false = login attempt failed, null = dismissed. + if (result == false && mounted) { + final authProvider = Provider.of(context, listen: false); + ScaffoldMessenger.of(context).showSnackBar( + SnackBar( + content: Text( + authProvider.errorMessage ?? + AppLocalizations.of(context).loginApiKeyInvalid, + ), + backgroundColor: Theme.of(context).colorScheme.error, + ), + ); + } } Future _handleLogin() async { @@ -538,3 +462,101 @@ class _LoginScreenState extends State { ); } } + +/// API-key login dialog. Owns its [TextEditingController] so it is disposed +/// with the dialog's State — never leaked (barrier/back dismissal) and never +/// disposed while the field is still mounted. Pops `true` on a successful +/// sign-in, `false` on a failed attempt, and `null` when dismissed. +@visibleForTesting +class ApiKeyLoginDialog extends StatefulWidget { + const ApiKeyLoginDialog({super.key, this.controller}); + + /// Test seam: a controller the dialog will adopt and dispose, so a test can + /// assert disposal. Production passes none and the dialog creates its own. + @visibleForTesting + final TextEditingController? controller; + + @override + State createState() => _ApiKeyLoginDialogState(); +} + +class _ApiKeyLoginDialogState extends State { + late final TextEditingController _apiKeyController = + widget.controller ?? TextEditingController(); + bool _isLoading = false; + + @override + void initState() { + super.initState(); + _apiKeyController.addListener(_onTextChanged); + } + + void _onTextChanged() => setState(() {}); + + @override + void dispose() { + _apiKeyController.removeListener(_onTextChanged); + _apiKeyController.dispose(); + super.dispose(); + } + + Future _submit() async { + final apiKey = _apiKeyController.text.trim(); + if (apiKey.isEmpty) return; + + setState(() => _isLoading = true); + + final authProvider = Provider.of(context, listen: false); + final success = await authProvider.loginWithApiKey(apiKey: apiKey); + + if (!mounted) return; + Navigator.of(context).pop(success); + } + + @override + Widget build(BuildContext context) { + final l = AppLocalizations.of(context); + return AlertDialog( + title: Text(l.loginApiKeyDialogTitle), + content: Column( + mainAxisSize: MainAxisSize.min, + children: [ + Text( + l.loginApiKeyDialogBody, + style: Theme.of(context).textTheme.bodyMedium?.copyWith( + color: Theme.of(context).colorScheme.onSurfaceVariant, + ), + ), + const SizedBox(height: 16), + TextField( + controller: _apiKeyController, + decoration: InputDecoration( + labelText: l.loginApiKeyLabel, + prefixIcon: const Icon(Icons.vpn_key_outlined), + ), + obscureText: true, + maxLines: 1, + enabled: !_isLoading, + onSubmitted: (_) => _submit(), + ), + ], + ), + actions: [ + TextButton( + onPressed: _isLoading ? null : () => Navigator.of(context).pop(), + child: Text(l.commonCancel), + ), + ElevatedButton( + onPressed: _isLoading || _apiKeyController.text.trim().isEmpty ? null : _submit, + child: _isLoading + ? const SizedBox( + height: 20, + width: 20, + child: CircularProgressIndicator(strokeWidth: 2), + ) + : Text(l.loginApiKeySignIn), + ), + ], + ); + } +} diff --git a/mobile/test/widgets/api_key_login_dialog_test.dart b/mobile/test/widgets/api_key_login_dialog_test.dart new file mode 100644 index 000000000..c90e4ab0e --- /dev/null +++ b/mobile/test/widgets/api_key_login_dialog_test.dart @@ -0,0 +1,75 @@ +import 'package:flutter/material.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:sure_mobile/l10n/app_localizations.dart'; +import 'package:sure_mobile/screens/login_screen.dart'; + +void main() { + // Opens the dialog with an injected controller and returns it so the test can + // assert the dialog disposed it. A disposed ChangeNotifier throws when used + // again, which is how we verify disposal. + Future openDialog(WidgetTester tester) async { + final controller = TextEditingController(); + await tester.pumpWidget( + MaterialApp( + localizationsDelegates: AppLocalizations.localizationsDelegates, + supportedLocales: AppLocalizations.supportedLocales, + home: Scaffold( + body: Builder( + builder: (context) => ElevatedButton( + onPressed: () => showDialog( + context: context, + builder: (_) => ApiKeyLoginDialog(controller: controller), + ), + child: const Text('open'), + ), + ), + ), + ), + ); + await tester.tap(find.text('open')); + await tester.pumpAndSettle(); + expect(find.text('API Key Login'), findsOneWidget); + return controller; + } + + void expectDisposed(TextEditingController controller) { + expect(() => controller.addListener(() {}), throwsA(isA())); + } + + testWidgets('disposes its controller when cancelled', (tester) async { + final controller = await openDialog(tester); + + await tester.tap(find.text('Cancel')); + await tester.pumpAndSettle(); + + expect(find.text('API Key Login'), findsNothing); + expectDisposed(controller); + }); + + testWidgets('disposes its controller when dismissed by a barrier tap', + (tester) async { + final controller = await openDialog(tester); + + // The barrier fills the screen but its centre is occluded by the dialog, so + // tap halfway between the screen corner and the dialog's top-left — a point + // derived from the dialog's real geometry (not a fixed coordinate) that is + // always on the dismissible barrier. + final dialogTopLeft = tester.getTopLeft(find.byType(AlertDialog)); + await tester.tapAt(dialogTopLeft / 2); + await tester.pumpAndSettle(); + + expect(find.text('API Key Login'), findsNothing); + expectDisposed(controller); + }); + + testWidgets('disposes its controller when dismissed by the system back button', + (tester) async { + final controller = await openDialog(tester); + + await tester.binding.handlePopRoute(); + await tester.pumpAndSettle(); + + expect(find.text('API Key Login'), findsNothing); + expectDisposed(controller); + }); +}