From f4994519bc87f07610eb6d38802a84bb19b9e142 Mon Sep 17 00:00:00 2001 From: PianoNic <79938743+Pianonic@users.noreply.github.com> Date: Wed, 12 Aug 2026 21:00:39 +0200 Subject: [PATCH] Report a failed request once, in words Keep the global toast quiet when a screen renders the server's own message, so a failed connect no longer shows a vague 'Request failed (400)' beside the text that says what to do. Give error toasts the destructive colour so they read as errors, and turn response bodies into a sentence instead of printing the map. --- lib/services/api_client.dart | 17 +++++- lib/services/api_error.dart | 60 ++++++++++++++++++++++ lib/services/toast_service.dart | 45 +++++++++++----- lib/ui/account/account_page.dart | 13 +++-- lib/ui/account/unified_connect_screen.dart | 8 ++- lib/ui/private/private_connect_screen.dart | 7 +-- 6 files changed, 121 insertions(+), 29 deletions(-) create mode 100644 lib/services/api_error.dart diff --git a/lib/services/api_client.dart b/lib/services/api_client.dart index 3bf9a02..3af59ae 100644 --- a/lib/services/api_client.dart +++ b/lib/services/api_client.dart @@ -50,6 +50,16 @@ class ApiClient { } static final ApiClient instance = ApiClient._(); + + /// Mark a request whose failure the caller renders itself, so the global + /// interceptor stays quiet instead of adding a second, vaguer message: + /// `Options(extra: {ApiClient.handlesErrors: true})`. + static const handlesErrors = '_handlesErrors'; + + /// Convenience for the above. + static Options handled([Options? options]) => + (options ?? Options()).copyWith(extra: {...?options?.extra, handlesErrors: true}); + late final Dio _dio; late final SchulyApi api; @@ -64,10 +74,13 @@ class ApiClient { } void _toastHttpError(DioException e) { + // The caller is showing this failure itself, with the server's own wording. + // Toasting "Request failed (400)" on top of that adds a second, vaguer copy. + if (e.requestOptions.extra[ApiClient.handlesErrors] == true) return; + final code = e.response?.statusCode; - final r = e.requestOptions; if (code != null) { - ToastService.error('Request failed ($code)', '${r.method} ${r.uri.path}'); + ToastService.error('Request failed ($code)', e); } else { ToastService.error('Network error', "Couldn't reach the server."); } diff --git a/lib/services/api_error.dart b/lib/services/api_error.dart new file mode 100644 index 0000000..c08de6b --- /dev/null +++ b/lib/services/api_error.dart @@ -0,0 +1,60 @@ +import 'package:dio/dio.dart'; + +/// Turns whatever the backend (or a plugin) returned into one sentence a person +/// can act on. Screens render this; nobody should be shown a raw response body. +class ApiError { + ApiError._(); + + static String describe(Object error) { + if (error is DioException) { + final fromBody = _fromBody(error.response?.data); + if (fromBody != null) return fromBody; + if (error.response?.statusCode != null) return _fromStatus(error.response!.statusCode!); + return "Couldn't reach the server. Check your connection and try again."; + } + return _tidy(error.toString()); + } + + static String? _fromBody(Object? data) { + if (data is Map) { + for (final key in const ['message', 'detail', 'title', 'error_description', 'error']) { + final value = data[key]; + if (value is String && value.trim().isNotEmpty) return _tidy(value); + } + } + if (data is String && data.trim().isNotEmpty && !data.trimLeft().startsWith('<')) { + return _tidy(data); + } + return null; + } + + static String _fromStatus(int status) => switch (status) { + 400 => "That didn't work. Check the details you entered and try again.", + 401 => 'Your session has expired. Sign in again.', + 403 => "You don't have access to this.", + 404 => "We couldn't find that.", + 408 || 504 => 'The server took too long to answer. Try again.', + 429 => 'Too many attempts. Wait a moment and try again.', + >= 500 => 'The server ran into a problem. Try again shortly.', + _ => 'Something went wrong (error $status).', + }; + + /// Backends sometimes prefix a message with a heading it already contains, + /// giving "MFA required: MFA required but ...". Collapse that, plus the + /// doubled punctuation and whitespace it leaves behind. + static String _tidy(String raw) { + var s = raw.replaceAll(RegExp(r'\s+'), ' ').trim(); + if (s.startsWith('Exception: ')) s = s.substring(11); + + final colon = s.indexOf(': '); + if (colon > 0) { + final head = s.substring(0, colon).trim(); + final tail = s.substring(colon + 2).trim(); + if (tail.toLowerCase().startsWith(head.toLowerCase())) s = tail; + } + + s = s.replaceAll(RegExp(r'\.{2,}'), '.').replaceAll(' .', '.').trim(); + if (s.isNotEmpty && !s.endsWith('.') && !s.endsWith('!') && !s.endsWith('?')) s = '$s.'; + return s.isEmpty ? 'Something went wrong.' : s; + } +} diff --git a/lib/services/toast_service.dart b/lib/services/toast_service.dart index 7fa54d2..81f7890 100644 --- a/lib/services/toast_service.dart +++ b/lib/services/toast_service.dart @@ -1,6 +1,8 @@ import 'package:flutter/widgets.dart'; import 'package:forui/forui.dart'; +import 'api_error.dart'; + /// App-wide toasts. [navigatorKey] is attached to the root `MaterialApp`, and the /// root `FToaster` wraps the navigator - so its context can surface a toast from /// anywhere, including services that have no `BuildContext`. Use this for errors @@ -13,30 +15,49 @@ class ToastService { static final GlobalKey navigatorKey = GlobalKey(); - static void error(String title, [Object? detail]) => - _show(title, _clean(detail), FIcons.circleAlert); + /// Errors carry the destructive colour and stay longer: they are read, not glanced at. + /// A toast is a glance, so the text is clipped here - a screen that can show the + /// full message inline should do that instead of relying on this. + static void error(String title, [Object? detail]) => _show( + title, + detail == null ? null : _clip(ApiError.describe(detail)), + FIcons.circleAlert, + severity: _Severity.error, + duration: const Duration(seconds: 8), + ); + + static String _clip(String s) => s.length > 140 ? '${s.substring(0, 139)}…' : s; static void success(String title, [String? description]) => - _show(title, description, FIcons.circleCheck); + _show(title, description, FIcons.circleCheck, severity: _Severity.success); static void info(String title, [String? description]) => - _show(title, description, FIcons.info); + _show(title, description, FIcons.info, severity: _Severity.info); - static void _show(String title, String? description, IconData icon) { + static void _show(String title, String? description, IconData icon, {required _Severity severity, Duration? duration}) { final context = navigatorKey.currentContext; if (context == null) return; + final colors = context.theme.colors; showFToast( context: context, icon: Icon(icon), title: Text(title), description: description == null || description.isEmpty ? null : Text(description), + duration: duration ?? const Duration(seconds: 5), + style: (style) => switch (severity) { + _Severity.error => style.copyWith( + decoration: style.decoration.copyWith( + color: colors.destructive, + border: Border.all(color: colors.destructive), + ), + iconStyle: style.iconStyle.copyWith(color: colors.destructiveForeground), + titleTextStyle: style.titleTextStyle.copyWith(color: colors.destructiveForeground), + descriptionTextStyle: style.descriptionTextStyle.copyWith(color: colors.destructiveForeground), + ), + _ => style, + }, ); } - - static String? _clean(Object? detail) { - if (detail == null) return null; - var s = detail.toString().replaceAll(RegExp(r'\s+'), ' ').trim(); - if (s.startsWith('Exception: ')) s = s.substring(11); - return s.length > 140 ? '${s.substring(0, 139)}…' : s; - } } + +enum _Severity { error, success, info } diff --git a/lib/ui/account/account_page.dart b/lib/ui/account/account_page.dart index a713e39..3a9d547 100644 --- a/lib/ui/account/account_page.dart +++ b/lib/ui/account/account_page.dart @@ -1,4 +1,3 @@ -import 'package:dio/dio.dart'; import 'package:flutter/material.dart'; import 'package:forui/forui.dart'; import 'package:schuly_api/schuly_api.dart'; @@ -7,6 +6,7 @@ import 'package:url_launcher/url_launcher.dart'; import '../../config/oidc_config.dart'; import '../../services/active_account_service.dart'; import '../../services/api_client.dart'; +import '../../services/api_error.dart'; import '../../services/school_data_service.dart'; import '../../services/toast_service.dart'; import '../classes/class_detail_screen.dart'; @@ -86,15 +86,18 @@ class _AccountPageState extends State { _syncMsg = null; }); try { - await ApiClient.instance.dio.post('$base/accounts/$accountId/sync'); + await ApiClient.instance.dio.post( + '$base/accounts/$accountId/sync', + options: ApiClient.handled(), + ); await SchoolDataService.instance.refresh(); await _loadSyncStatus(); if (mounted) setState(() => _syncMsg = 'Synced just now'); ToastService.success('Synced', 'Fetched fresh data from the provider.'); - } on DioException catch (e) { - if (mounted) setState(() => _syncMsg = 'Sync failed (${e.response?.statusCode ?? 'network'})'); } catch (e) { - if (mounted) setState(() => _syncMsg = 'Sync failed'); + // One path for every failure: the reason on the row, and a toast because + // the user may have scrolled away from it. + if (mounted) setState(() => _syncMsg = ApiError.describe(e)); ToastService.error('Sync failed', e); } finally { if (mounted) setState(() => _syncing = false); diff --git a/lib/ui/account/unified_connect_screen.dart b/lib/ui/account/unified_connect_screen.dart index 2fd5052..4526ff6 100644 --- a/lib/ui/account/unified_connect_screen.dart +++ b/lib/ui/account/unified_connect_screen.dart @@ -1,9 +1,9 @@ -import 'package:dio/dio.dart'; import 'package:flutter/material.dart'; import 'package:forui/forui.dart'; import '../../domain/school_system.dart'; import '../../services/api_client.dart'; +import '../../services/api_error.dart'; import '../widgets/dynamic_login_form.dart'; class UnifiedConnectScreen extends StatefulWidget { @@ -58,14 +58,12 @@ class _UnifiedConnectScreenState extends State { 'fields': fields, 'displayName': name.isEmpty ? _system.displayName : name, }, + options: ApiClient.handled(), ); final accountId = res.data?['accountId']?.toString(); if (mounted) Navigator.of(context).pop(accountId); - } on DioException catch (e) { - setState(() => _error = - 'HTTP ${e.response?.statusCode ?? '?'}: ${e.response?.data}'); } catch (e) { - setState(() => _error = '$e'); + setState(() => _error = ApiError.describe(e)); } finally { if (mounted) setState(() => _busy = false); } diff --git a/lib/ui/private/private_connect_screen.dart b/lib/ui/private/private_connect_screen.dart index 79f94ad..a6d5026 100644 --- a/lib/ui/private/private_connect_screen.dart +++ b/lib/ui/private/private_connect_screen.dart @@ -1,4 +1,3 @@ -import 'package:dio/dio.dart'; import 'package:flutter/material.dart'; import 'package:forui/forui.dart'; @@ -6,6 +5,7 @@ import '../../domain/school_system.dart'; import '../../services/private_account_store.dart'; import '../../services/scrape_proxy_client.dart'; import '../../services/token_proxy_client.dart'; +import '../../services/api_error.dart'; import '../../services/totp_service.dart'; import '../widgets/dynamic_login_form.dart'; @@ -66,11 +66,8 @@ class _PrivateConnectScreenState extends State { } else { await _connectScrape(baseUrl, name, basePath); } - } on DioException catch (e) { - setState(() => _error = - 'HTTP ${e.response?.statusCode ?? '?'}: ${e.response?.data}'); } catch (e) { - setState(() => _error = '$e'); + setState(() => _error = ApiError.describe(e)); } finally { if (mounted) setState(() => _busy = false); }