From d2cca1da82dd8509d8f4ae054123a3846f176d7a Mon Sep 17 00:00:00 2001 From: Strycher Date: Sat, 20 Jun 2026 03:18:55 -0400 Subject: [PATCH] fix(#28): guard settings onChanged persisters against silent failures (panes) Settings onChanged/onTap callbacks fire-and-forgot AppSettingsService Future setters, so a persist failure rejected unhandled and was swallowed. Add a shared persistSetting(context, action) helper that awaits the setter in a try/catch and shows a dismissible error SnackBar, capturing the messenger before the await so it survives a picker-dialog pop. Wrap the 27 onChanged/onTap setter sites in app_settings_view + message_settings_view; setAppDebugLogEnabled and setNotificationsEnabled (which await + show their own confirmation) get an inline try/catch instead. Covers the two onChanged panes; the settings_screen action flows (GPX export, location dialogs, version-info mounted guard) follow in a second pass, so #28 stays open. Error strings are English for now (separate settings l10n sweep). Adds a persistSetting widget test. Co-Authored-By: Claude Opus 4.8 --- lib/helpers/settings_persist.dart | 36 +++++++++++ lib/screens/settings/app_settings_view.dart | 64 ++++++++++++------- .../settings/message_settings_view.dart | 59 ++++++++++++----- test/helpers/settings_persist_test.dart | 47 ++++++++++++++ 4 files changed, 167 insertions(+), 39 deletions(-) create mode 100644 lib/helpers/settings_persist.dart create mode 100644 test/helpers/settings_persist_test.dart diff --git a/lib/helpers/settings_persist.dart b/lib/helpers/settings_persist.dart new file mode 100644 index 0000000..7c0da55 --- /dev/null +++ b/lib/helpers/settings_persist.dart @@ -0,0 +1,36 @@ +import 'package:flutter/material.dart'; + +/// Runs a settings-persist [action] and surfaces a dismissible error SnackBar if +/// it throws, instead of dropping the returned Future and swallowing the error +/// (no silent failures — #28). +/// +/// Call sites intentionally don't await the returned Future: this function never +/// rethrows (it catches internally), so dropping it is safe. The +/// [ScaffoldMessenger] is captured before the await, so the error still shows +/// even when the triggering widget (e.g. a picker dialog) is popped immediately +/// after. +/// +/// [message] is English-only for now; localizing it is part of the separate +/// settings l10n sweep noted on #28. +Future persistSetting( + BuildContext context, + Future Function() action, { + String message = 'Could not save that setting. Please try again.', +}) async { + final messenger = ScaffoldMessenger.of(context); + try { + await action(); + } catch (_) { + messenger.showSnackBar( + SnackBar( + content: GestureDetector( + onTap: () => messenger.hideCurrentSnackBar(), + child: Text(message), + ), + behavior: SnackBarBehavior.floating, + duration: const Duration(seconds: 4), + dismissDirection: DismissDirection.down, + ), + ); + } +} diff --git a/lib/screens/settings/app_settings_view.dart b/lib/screens/settings/app_settings_view.dart index c238857..247ac54 100644 --- a/lib/screens/settings/app_settings_view.dart +++ b/lib/screens/settings/app_settings_view.dart @@ -9,6 +9,7 @@ import '../../models/app_settings.dart'; import '../../models/translation_support.dart'; import '../../services/app_settings_service.dart'; import '../../services/translation_service.dart'; +import '../../helpers/settings_persist.dart'; import '../../helpers/snack_bar_builder.dart'; import '../map_cache_screen.dart'; @@ -106,7 +107,7 @@ class AppSettingsView extends StatelessWidget { ), value: settingsService.settings.enableMessageTracing, onChanged: (value) { - settingsService.setEnableMessageTracing(value); + persistSetting(context, () => settingsService.setEnableMessageTracing(value)); }, ), ], @@ -135,7 +136,7 @@ class AppSettingsView extends StatelessWidget { subtitle: Text(context.l10n.appSettings_showRepeatersSubtitle), value: settingsService.settings.mapShowRepeaters, onChanged: (value) { - settingsService.setMapShowRepeaters(value); + persistSetting(context, () => settingsService.setMapShowRepeaters(value)); }, ), const Divider(height: 1), @@ -145,7 +146,7 @@ class AppSettingsView extends StatelessWidget { subtitle: Text(context.l10n.appSettings_showChatNodesSubtitle), value: settingsService.settings.mapShowChatNodes, onChanged: (value) { - settingsService.setMapShowChatNodes(value); + persistSetting(context, () => settingsService.setMapShowChatNodes(value)); }, ), const Divider(height: 1), @@ -155,7 +156,7 @@ class AppSettingsView extends StatelessWidget { subtitle: Text(context.l10n.appSettings_showOtherNodesSubtitle), value: settingsService.settings.mapShowOtherNodes, onChanged: (value) { - settingsService.setMapShowOtherNodes(value); + persistSetting(context, () => settingsService.setMapShowOtherNodes(value)); }, ), const Divider(height: 1), @@ -232,7 +233,7 @@ class AppSettingsView extends StatelessWidget { title: Text(context.l10n.translation_enableTitle), subtitle: Text(context.l10n.translation_enableSubtitle), value: settings.translationEnabled, - onChanged: settingsService.setTranslationEnabled, + onChanged: (value) => persistSetting(context, () => settingsService.setTranslationEnabled(value)), ), const Divider(height: 1), SwitchListTile( @@ -250,7 +251,7 @@ class AppSettingsView extends StatelessWidget { ), value: settings.autoTranslateIncomingMessages, onChanged: translationEnabled - ? settingsService.setAutoTranslateIncomingMessages + ? (value) => persistSetting(context, () => settingsService.setAutoTranslateIncomingMessages(value)) : null, ), const Divider(height: 1), @@ -269,7 +270,7 @@ class AppSettingsView extends StatelessWidget { ), value: settings.composerTranslationEnabled, onChanged: translationEnabled - ? settingsService.setComposerTranslationEnabled + ? (value) => persistSetting(context, () => settingsService.setComposerTranslationEnabled(value)) : null, ), const Divider(height: 1), @@ -306,7 +307,7 @@ class AppSettingsView extends StatelessWidget { onChanged: settings.translationDownloadedModels.isEmpty ? null : (value) { - settingsService.setTranslationSelectedModelId(value); + persistSetting(context, () => settingsService.setTranslationSelectedModelId(value)); }, ), ), @@ -350,7 +351,7 @@ class AppSettingsView extends StatelessWidget { children: [ _TranslationUrlField( initialValue: settings.translationModelSourceUrl ?? '', - onChanged: settingsService.setTranslationModelSourceUrl, + onChanged: (value) => persistSetting(context, () => settingsService.setTranslationModelSourceUrl(value)), onDownload: translationService.isBusy ? null : (url) => _downloadTranslationModel( @@ -423,8 +424,12 @@ class AppSettingsView extends StatelessWidget { ), icon: const Icon(Icons.delete_outline), ), - onTap: () => settingsService - .setTranslationSelectedModelId(model.id), + onTap: () => persistSetting( + context, + () => settingsService.setTranslationSelectedModelId( + model.id, + ), + ), ), ), ], @@ -497,9 +502,12 @@ class AppSettingsView extends StatelessWidget { onChanged: isConnected ? (value) { if (value != null) { - settingsService.setBatteryChemistryForDevice( - deviceId, - value, + persistSetting( + context, + () => settingsService.setBatteryChemistryForDevice( + deviceId, + value, + ), ); } } @@ -537,7 +545,7 @@ class AppSettingsView extends StatelessWidget { groupValue: settingsService.settings.themeMode, onChanged: (value) { if (value != null) { - settingsService.setThemeMode(value); + persistSetting(context, () => settingsService.setThemeMode(value)); Navigator.pop(context); } }, @@ -592,7 +600,7 @@ class AppSettingsView extends StatelessWidget { groupValue: settingsService.settings.clockFormat, onChanged: (value) { if (value != null) { - settingsService.setClockFormat(value); + persistSetting(context, () => settingsService.setClockFormat(value)); Navigator.pop(context); } }, @@ -690,7 +698,7 @@ class AppSettingsView extends StatelessWidget { child: RadioGroup( groupValue: settingsService.settings.languageOverride, onChanged: (value) { - settingsService.setLanguageOverride(value); + persistSetting(context, () => settingsService.setLanguageOverride(value)); Navigator.pop(context); }, child: Column( @@ -798,7 +806,7 @@ class AppSettingsView extends StatelessWidget { groupValue: settingsService.settings.mapTimeFilterHours, onChanged: (value) { if (value != null) { - settingsService.setMapTimeFilterHours(value); + persistSetting(context, () => settingsService.setMapTimeFilterHours(value)); Navigator.pop(context); } }, @@ -852,7 +860,7 @@ class AppSettingsView extends StatelessWidget { groupValue: settingsService.settings.unitSystem, onChanged: (value) { if (value != null) { - settingsService.setUnitSystem(value); + persistSetting(context, () => settingsService.setUnitSystem(value)); Navigator.pop(context); } }, @@ -890,7 +898,7 @@ class AppSettingsView extends StatelessWidget { currentLanguageCode: settingsService.settings.translationTargetLanguageCode, onLanguageSelected: (value) { - settingsService.setTranslationTargetLanguageCode(value); + persistSetting(context, () => settingsService.setTranslationTargetLanguageCode(value)); Navigator.pop(context); }, ), @@ -1037,7 +1045,7 @@ class AppSettingsView extends StatelessWidget { }).toList(), onChanged: (value) { if (value != null) { - settingsService.setSelectedCyr2LatProfile(value); + persistSetting(context, () => settingsService.setSelectedCyr2LatProfile(value)); } }, ), @@ -1318,7 +1326,19 @@ class AppSettingsView extends StatelessWidget { subtitle: Text(context.l10n.appSettings_appDebugLoggingSubtitle), value: settingsService.settings.appDebugLogEnabled, onChanged: (value) async { - await settingsService.setAppDebugLogEnabled(value); + try { + await settingsService.setAppDebugLogEnabled(value); + } catch (_) { + if (context.mounted) { + showDismissibleSnackBar( + context, + content: const Text( + 'Could not change debug logging. Please try again.', + ), + ); + } + return; + } if (!context.mounted) return; showDismissibleSnackBar( context, diff --git a/lib/screens/settings/message_settings_view.dart b/lib/screens/settings/message_settings_view.dart index 8ca3820..50ff4cd 100644 --- a/lib/screens/settings/message_settings_view.dart +++ b/lib/screens/settings/message_settings_view.dart @@ -4,6 +4,7 @@ import 'package:provider/provider.dart'; import '../../l10n/l10n.dart'; import '../../services/app_settings_service.dart'; import '../../services/notification_service.dart'; +import '../../helpers/settings_persist.dart'; import '../../helpers/snack_bar_builder.dart'; /// Embeddable view (no Scaffold) for the Message Settings shell pane. @@ -70,7 +71,19 @@ class MessageSettingsView extends StatelessWidget { } } - await settingsService.setNotificationsEnabled(value); + try { + await settingsService.setNotificationsEnabled(value); + } catch (_) { + if (context.mounted) { + showDismissibleSnackBar( + context, + content: const Text( + 'Could not change notifications. Please try again.', + ), + ); + } + return; + } if (context.mounted) { showDismissibleSnackBar( context, @@ -111,7 +124,7 @@ class MessageSettingsView extends StatelessWidget { value: settingsService.settings.notifyOnNewMessage, onChanged: settingsService.settings.notificationsEnabled ? (value) { - settingsService.setNotifyOnNewMessage(value); + persistSetting(context, () => settingsService.setNotifyOnNewMessage(value)); } : null, ), @@ -142,7 +155,7 @@ class MessageSettingsView extends StatelessWidget { value: settingsService.settings.notifyOnNewChannelMessage, onChanged: settingsService.settings.notificationsEnabled ? (value) { - settingsService.setNotifyOnNewChannelMessage(value); + persistSetting(context, () => settingsService.setNotifyOnNewChannelMessage(value)); } : null, ), @@ -173,7 +186,7 @@ class MessageSettingsView extends StatelessWidget { value: settingsService.settings.notifyOnNewAdvert, onChanged: settingsService.settings.notificationsEnabled ? (value) { - settingsService.setNotifyOnNewAdvert(value); + persistSetting(context, () => settingsService.setNotifyOnNewAdvert(value)); } : null, ), @@ -205,7 +218,7 @@ class MessageSettingsView extends StatelessWidget { ), value: settingsService.settings.clearPathOnMaxRetry, onChanged: (value) { - settingsService.setClearPathOnMaxRetry(value); + persistSetting(context, () => settingsService.setClearPathOnMaxRetry(value)); showDismissibleSnackBar( context, content: Text( @@ -223,7 +236,7 @@ class MessageSettingsView extends StatelessWidget { title: Text(context.l10n.appSettings_jumpToOldestUnread), subtitle: Text(context.l10n.appSettings_jumpToOldestUnreadSubtitle), value: settingsService.settings.jumpToOldestUnread, - onChanged: settingsService.setJumpToOldestUnread, + onChanged: (value) => persistSetting(context, () => settingsService.setJumpToOldestUnread(value)), ), const Divider(height: 1), SwitchListTile( @@ -232,7 +245,7 @@ class MessageSettingsView extends StatelessWidget { subtitle: Text(context.l10n.appSettings_autoRouteRotationSubtitle), value: settingsService.settings.autoRouteRotationEnabled, onChanged: (value) { - settingsService.setAutoRouteRotationEnabled(value); + persistSetting(context, () => settingsService.setAutoRouteRotationEnabled(value)); showDismissibleSnackBar( context, content: Text( @@ -260,8 +273,10 @@ class MessageSettingsView extends StatelessWidget { label: settingsService.settings.maxRouteWeight .round() .toString(), - onChanged: (value) => - settingsService.setMaxRouteWeight(value), + onChanged: (value) => persistSetting( + context, + () => settingsService.setMaxRouteWeight(value), + ), ), ], ), @@ -280,8 +295,10 @@ class MessageSettingsView extends StatelessWidget { divisions: 9, label: settingsService.settings.initialRouteWeight .toStringAsFixed(1), - onChanged: (value) => - settingsService.setInitialRouteWeight(value), + onChanged: (value) => persistSetting( + context, + () => settingsService.setInitialRouteWeight(value), + ), ), ], ), @@ -304,8 +321,11 @@ class MessageSettingsView extends StatelessWidget { divisions: 19, label: settingsService.settings.routeWeightSuccessIncrement .toStringAsFixed(1), - onChanged: (value) => - settingsService.setRouteWeightSuccessIncrement(value), + onChanged: (value) => persistSetting( + context, + () => + settingsService.setRouteWeightSuccessIncrement(value), + ), ), ], ), @@ -328,8 +348,11 @@ class MessageSettingsView extends StatelessWidget { divisions: 19, label: settingsService.settings.routeWeightFailureDecrement .toStringAsFixed(1), - onChanged: (value) => - settingsService.setRouteWeightFailureDecrement(value), + onChanged: (value) => persistSetting( + context, + () => + settingsService.setRouteWeightFailureDecrement(value), + ), ), ], ), @@ -349,8 +372,10 @@ class MessageSettingsView extends StatelessWidget { divisions: 8, label: settingsService.settings.maxMessageRetries .toString(), - onChanged: (value) => - settingsService.setMaxMessageRetries(value.toInt()), + onChanged: (value) => persistSetting( + context, + () => settingsService.setMaxMessageRetries(value.toInt()), + ), ), ], ), diff --git a/test/helpers/settings_persist_test.dart b/test/helpers/settings_persist_test.dart new file mode 100644 index 0000000..9c13ac0 --- /dev/null +++ b/test/helpers/settings_persist_test.dart @@ -0,0 +1,47 @@ +import 'package:flutter/material.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:meshcore_open/helpers/settings_persist.dart'; + +void main() { + Widget harness(Future Function() action) { + return MaterialApp( + home: Scaffold( + body: Builder( + builder: (context) => ElevatedButton( + onPressed: () => + persistSetting(context, action, message: 'Save failed'), + child: const Text('go'), + ), + ), + ), + ); + } + + testWidgets('shows an error SnackBar when the persist action throws', ( + tester, + ) async { + await tester.pumpWidget( + harness(() async { + throw Exception('boom'); + }), + ); + + await tester.tap(find.text('go')); + await tester.pump(); // run persistSetting (catch + schedule SnackBar) + await tester.pump(); // build the SnackBar + + expect(find.text('Save failed'), findsOneWidget); + }); + + testWidgets('shows no SnackBar when the persist action succeeds', ( + tester, + ) async { + await tester.pumpWidget(harness(() async {})); + + await tester.tap(find.text('go')); + await tester.pump(); + await tester.pump(); + + expect(find.byType(SnackBar), findsNothing); + }); +}