Better error handling

We can now distinguish between errors that should block the application, (hopefully)
transient ones that only warrant a snackbar and others that are only logged.

Author: rolandgeider

lib/helpers/errors.dart

@@ -23,6 +23,7 @@ import 'package:flutter/foundation.dart';
 import 'package:flutter/material.dart';
 import 'package:flutter/services.dart';
 import 'package:flutter_html/flutter_html.dart';
+import 'package:http/http.dart' as http;
 import 'package:json_annotation/json_annotation.dart';
 import 'package:logging/logging.dart';
 import 'package:provider/provider.dart';
@@ -42,6 +43,18 @@ import 'logs.dart';
 /// modal dialogs on top of each other.
 bool _errorDialogVisible = false;
 
+/// How an error should be surfaced to the user.
+enum ErrorSeverity {
+  /// Logged only; not worth interrupting the user (e.g. layout overflows).
+  cosmetic,
+
+  /// A brief, non-blocking snackbar (e.g. transient connectivity problems).
+  transient,
+
+  /// A blocking error dialog.
+  fatal,
+}
+
 void showHttpExceptionErrorDialog(WgerHttpException exception, {BuildContext? context}) {
   final logger = Logger('showHttpExceptionErrorDialog');
 
@@ -97,7 +110,7 @@ void showGeneralErrorDialog(dynamic error, StackTrace? stackTrace, {BuildContext
   // of a widget's build method.
   final BuildContext? dialogContext = context ?? navigatorKey.currentContext;
 
-  final logger = Logger('showHttpExceptionErrorDialog');
+  final logger = Logger('showGeneralErrorDialog');
 
   if (dialogContext == null) {
     if (kDebugMode) {
@@ -114,30 +127,16 @@ void showGeneralErrorDialog(dynamic error, StackTrace? stackTrace, {BuildContext
 
   final i18n = AppLocalizations.of(dialogContext);
 
-  // If possible, determine the error title and message based on the error type.
+  // If possible, determine the issue title and message based on the error type.
   // (Note that issue titles and error messages are not localized)
-  bool allowReportIssue = true;
   String issueTitle = 'An error occurred';
   String issueErrorMessage = error.toString();
-  String errorTitle = i18n.anErrorOccurred;
-  String errorDescription = i18n.errorInfoDescription;
-  var icon = Icons.error;
-
-  if (error is TimeoutException) {
-    issueTitle = 'Network Timeout';
-    issueErrorMessage =
-        'The connection to the server timed out. Please check your '
-        'internet connection and try again.';
-  } else if (error is FlutterErrorDetails) {
+
+  if (error is FlutterErrorDetails) {
     issueTitle = 'Application Error';
     issueErrorMessage = error.exceptionAsString();
   } else if (error is MissingRequiredKeysException) {
     issueTitle = 'Missing Required Key';
-  } else if (error is SocketException) {
-    allowReportIssue = false;
-    icon = Icons.signal_wifi_connected_no_internet_4_outlined;
-    errorTitle = i18n.errorCouldNotConnectToServer;
-    errorDescription = i18n.errorCouldNotConnectToServerDetails;
   }
 
   final String fullStackTrace = stackTrace?.toString() ?? 'No stack trace available.';
@@ -152,16 +151,19 @@ void showGeneralErrorDialog(dynamic error, StackTrace? stackTrace, {BuildContext
           spacing: 8,
           mainAxisAlignment: MainAxisAlignment.center,
           children: [
-            Icon(icon, color: Theme.of(context).colorScheme.error),
+            Icon(Icons.error, color: Theme.of(context).colorScheme.error),
             Expanded(
-              child: Text(errorTitle, style: TextStyle(color: Theme.of(context).colorScheme.error)),
+              child: Text(
+                i18n.anErrorOccurred,
+                style: TextStyle(color: Theme.of(context).colorScheme.error),
+              ),
             ),
           ],
         ),
         content: SingleChildScrollView(
           child: ListBody(
             children: [
-              Text(errorDescription),
+              Text(i18n.errorInfoDescription),
               const SizedBox(height: 8),
               Text(i18n.errorInfoDescription2),
               const SizedBox(height: 10),
@@ -216,30 +218,29 @@ void showGeneralErrorDialog(dynamic error, StackTrace? stackTrace, {BuildContext
           ),
         ),
         actions: [
-          if (allowReportIssue)
-            TextButton(
-              child: const Text('Report issue'),
-              onPressed: () async {
-                final githubIssueUrl = buildGithubIssueUrl(
-                  issueTitle: issueTitle,
-                  issueErrorMessage: issueErrorMessage,
-                  stackTrace: fullStackTrace,
-                  applicationLogs: applicationLogs,
-                );
-                final Uri reportUri = Uri.parse(githubIssueUrl);
-
-                try {
-                  await launchUrl(reportUri, mode: LaunchMode.externalApplication);
-                } catch (e) {
-                  if (kDebugMode) {
-                    logger.warning('Error launching URL: $e');
-                  }
-                  ScaffoldMessenger.of(
-                    context,
-                  ).showSnackBar(SnackBar(content: Text('Error opening issue tracker: $e')));
+          TextButton(
+            child: const Text('Report issue'),
+            onPressed: () async {
+              final githubIssueUrl = buildGithubIssueUrl(
+                issueTitle: issueTitle,
+                issueErrorMessage: issueErrorMessage,
+                stackTrace: fullStackTrace,
+                applicationLogs: applicationLogs,
+              );
+              final Uri reportUri = Uri.parse(githubIssueUrl);
+
+              try {
+                await launchUrl(reportUri, mode: LaunchMode.externalApplication);
+              } catch (e) {
+                if (kDebugMode) {
+                  logger.warning('Error launching URL: $e');
                 }
-              },
-            ),
+                ScaffoldMessenger.of(
+                  context,
+                ).showSnackBar(SnackBar(content: Text('Error opening issue tracker: $e')));
+              }
+            },
+          ),
           FilledButton(
             child: Text(MaterialLocalizations.of(context).okButtonLabel),
             onPressed: () {
@@ -252,6 +253,61 @@ void showGeneralErrorDialog(dynamic error, StackTrace? stackTrace, {BuildContext
   ).whenComplete(() => _errorDialogVisible = false);
 }
 
+/// Classifies [error] to decide how it should be surfaced.
+ErrorSeverity classifyError(Object? error) {
+  // Flutter reports layout overflows as a plain FlutterError without a
+  // dedicated type, so matching the message is the only option.
+  final isLayoutOverflow = error is FlutterError && error.toString().contains('overflowed');
+
+  if (error is NetworkImageLoadException || isLayoutOverflow) {
+    return ErrorSeverity.cosmetic;
+  }
+  if (error is SocketException || error is http.ClientException || error is TimeoutException) {
+    return ErrorSeverity.transient;
+  }
+  return ErrorSeverity.fatal;
+}
+
+/// Routes [error] to the appropriate UI based on its [ErrorSeverity].
+///
+/// The caller is responsible for logging the error beforehand.
+void handleError(Object? error, StackTrace? stackTrace) {
+  switch (classifyError(error)) {
+    case ErrorSeverity.cosmetic:
+      break;
+    case ErrorSeverity.transient:
+      showTransientErrorSnackbar();
+    case ErrorSeverity.fatal:
+      if (error is WgerHttpException) {
+        showHttpExceptionErrorDialog(error);
+      } else {
+        showGeneralErrorDialog(error, stackTrace);
+      }
+  }
+}
+
+/// Shows a brief, non-blocking snackbar telling the user about a (hopefully)
+/// transient error such as network problems, etc.
+void showTransientErrorSnackbar() {
+  final messenger = scaffoldMessengerKey.currentState;
+  final context = navigatorKey.currentContext;
+
+  if (messenger == null || context == null) {
+    if (kDebugMode) {
+      Logger(
+        'showNetworkErrorSnackbar',
+      ).warning('Could not show snackbar: no messenger or context available.');
+    }
+    return;
+  }
+
+  messenger
+    ..clearSnackBars()
+    ..showSnackBar(
+      SnackBar(content: Text(AppLocalizations.of(context).errorCouldNotConnectToServer)),
+    );
+}
+
 /// Builds the URL that opens a pre-filled GitHub bug report.
 ///
 /// The error details are passed to GitHub as query parameters and since

lib/main.dart

@@ -21,7 +21,6 @@ import 'package:flutter/material.dart';
 import 'package:flutter_riverpod/flutter_riverpod.dart' as riverpod;
 import 'package:logging/logging.dart';
 import 'package:provider/provider.dart';
-import 'package:wger/core/exceptions/http_exception.dart';
 import 'package:wger/core/locator.dart';
 import 'package:wger/helpers/errors.dart';
 import 'package:wger/helpers/locale.dart';
@@ -82,6 +81,7 @@ void _setupLogging() {
 }
 
 final GlobalKey<NavigatorState> navigatorKey = GlobalKey<NavigatorState>();
+final GlobalKey<ScaffoldMessengerState> scaffoldMessengerKey = GlobalKey<ScaffoldMessengerState>();
 
 void main() async {
   // Needs to be called before runApp
@@ -97,26 +97,14 @@ void main() async {
 
   // SharedPreferences to SharedPreferencesAsync migration function
   await PreferenceHelper.instance.migrationSupportFunctionForSharedPreferences();
-  // Catch errors from Flutter itself (widget build, layout, paint, etc.)
-  //
-  // NOTE: it seems this sometimes makes problems and even freezes the flutter
-  //       process when widgets overflow, so it is disabled in dev mode.
-  if (!kDebugMode) {
-    FlutterError.onError = (FlutterErrorDetails details) {
-      final stack = details.stack ?? StackTrace.empty;
-      logger.severe('Error caught by FlutterError.onError: ${details.exception}');
-
-      FlutterError.dumpErrorToConsole(details);
 
-      // Don't show the full error dialog for network image loading errors.
-      if (details.exception is NetworkImageLoadException) {
-        return;
-      }
-
-      showGeneralErrorDialog(details.exception, stack);
-      // throw details.exception;
-    };
-  }
+  // Catch errors from Flutter itself (widget build, layout, paint, etc.)
+  FlutterError.onError = (FlutterErrorDetails details) {
+    final stack = details.stack ?? StackTrace.empty;
+    logger.severe('Error caught by FlutterError.onError: ${details.exception}');
+    FlutterError.dumpErrorToConsole(details);
+    handleError(details.exception, stack);
+  };
 
   // Catch errors that happen outside of the Flutter framework (e.g., in async operations)
   PlatformDispatcher.instance.onError = (error, stack) {
@@ -131,11 +119,7 @@ void main() async {
     logger.severe('Error caught by PlatformDispatcher.instance.onError: $error');
     logger.severe('Stack trace: $stack');
 
-    if (error is WgerHttpException) {
-      showHttpExceptionErrorDialog(error);
-    } else {
-      showGeneralErrorDialog(error, stack);
-    }
+    handleError(error, stack);
 
     // Return true to indicate that the error has been handled.
     return true;
@@ -244,6 +228,7 @@ class MainApp extends StatelessWidget {
                   return MaterialApp(
                     title: 'wger',
                     navigatorKey: navigatorKey,
+                    scaffoldMessengerKey: scaffoldMessengerKey,
                     theme: wgerLightTheme,
                     darkTheme: wgerDarkTheme,
                     highContrastTheme: wgerLightThemeHc,

test/helpers/errors_test.dart

@@ -16,7 +16,13 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+import 'dart:async';
+import 'dart:io';
+
+import 'package:flutter/material.dart';
 import 'package:flutter_test/flutter_test.dart';
+import 'package:http/http.dart' as http;
+import 'package:wger/core/exceptions/http_exception.dart';
 import 'package:wger/helpers/consts.dart';
 import 'package:wger/helpers/errors.dart';
 
@@ -178,4 +184,38 @@ void main() {
       expect(description, contains('#79 SomeClass.someMethod'));
     });
   });
+
+  group('classifyError', () {
+    test('Connectivity errors are transient', () {
+      expect(
+        classifyError(const SocketException('no route to host')),
+        ErrorSeverity.transient,
+      );
+      expect(
+        classifyError(http.ClientException('connection refused')),
+        ErrorSeverity.transient,
+      );
+      expect(classifyError(TimeoutException('request timed out')), ErrorSeverity.transient);
+    });
+
+    test('Network image and layout overflow errors are cosmetic', () {
+      expect(
+        classifyError(
+          NetworkImageLoadException(statusCode: 404, uri: Uri.parse('https://x/y.png')),
+        ),
+        ErrorSeverity.cosmetic,
+      );
+      expect(
+        classifyError(FlutterError('A RenderFlex overflowed by 99 pixels on the right.')),
+        ErrorSeverity.cosmetic,
+      );
+    });
+
+    test('Other errors are fatal', () {
+      expect(classifyError(WgerHttpException.fromMap({'detail': 'invalid'})), ErrorSeverity.fatal);
+      expect(classifyError(Exception('boom')), ErrorSeverity.fatal);
+      expect(classifyError(StateError('bad state')), ErrorSeverity.fatal);
+      expect(classifyError(null), ErrorSeverity.fatal);
+    });
+  });
 }