From ff320c17b6ee1314f48b0524f644aacd2e69224e Mon Sep 17 00:00:00 2001 From: Strycher Date: Sat, 8 Aug 2026 00:18:00 -0400 Subject: [PATCH] fix(#528): surface late repeater CLI replies instead of discarding them A repeater reply that arrived after its command's window closed hit `if (commandId.isEmpty) return;` in RepeaterCommandService.handleResponse and was dropped with no log and no UI. Since the command timeout can be shorter than the app's own message-retrieval budget, that made "the command ran but the response was never reported" the normal outcome rather than an edge case, and it affected every repeater screen, not just the CLI one. RepeaterCommandService now remembers a timed-out command's prefix for two minutes, so a reply arriving afterwards can be attributed to the request it answers. Replies that reach no waiting command are handed to a new onUnmatchedResponse sink and logged through appLogger. No path through handleResponse returns without either completing a command, surfacing the payload, or logging why it could not. The CLI screen renders these as a distinct history entry naming the original command and how late it was. The settings screen applies the value if it is a `get` reply and tells the user it arrived late. repeater_status_screen already parsed responses independently of the service, so it had no silent-loss path to fix. Part of epic #473. Does not change the timeout window itself (#529), the reported duration (#531), or the stale-prefix fallback (#532). --- lib/l10n/app_en.arb | 13 ++ lib/l10n/app_localizations.dart | 18 +++ lib/l10n/app_localizations_bg.dart | 13 ++ lib/l10n/app_localizations_de.dart | 13 ++ lib/l10n/app_localizations_en.dart | 13 ++ lib/l10n/app_localizations_es.dart | 13 ++ lib/l10n/app_localizations_fr.dart | 13 ++ lib/l10n/app_localizations_hu.dart | 13 ++ lib/l10n/app_localizations_it.dart | 13 ++ lib/l10n/app_localizations_ja.dart | 13 ++ lib/l10n/app_localizations_ko.dart | 13 ++ lib/l10n/app_localizations_nl.dart | 13 ++ lib/l10n/app_localizations_pl.dart | 13 ++ lib/l10n/app_localizations_pt.dart | 13 ++ lib/l10n/app_localizations_ru.dart | 13 ++ lib/l10n/app_localizations_sk.dart | 13 ++ lib/l10n/app_localizations_sl.dart | 13 ++ lib/l10n/app_localizations_sv.dart | 13 ++ lib/l10n/app_localizations_uk.dart | 13 ++ lib/l10n/app_localizations_zh.dart | 13 ++ lib/screens/repeater_cli_screen.dart | 90 ++++++++--- lib/screens/repeater_settings_screen.dart | 16 ++ lib/services/repeater_command_service.dart | 145 ++++++++++++++++-- .../repeater_command_service_test.dart | 117 ++++++++++++++ 24 files changed, 602 insertions(+), 31 deletions(-) create mode 100644 test/services/repeater_command_service_test.dart diff --git a/lib/l10n/app_en.arb b/lib/l10n/app_en.arb index 8da0d72..6e0de1c 100644 --- a/lib/l10n/app_en.arb +++ b/lib/l10n/app_en.arb @@ -1566,6 +1566,19 @@ } } }, + "repeater_cliLateResponse": "Late response to \"{command}\" (arrived {seconds}s after timeout)", + "@repeater_cliLateResponse": { + "placeholders": { + "command": { + "type": "String" + }, + "seconds": { + "type": "String" + } + } + }, + "repeater_cliUnmatchedResponse": "Unrequested response from repeater", + "repeater_lateResponseReceived": "A late response arrived after the command timed out. See the CLI screen.", "repeater_cliQuickGetName": "Get Name", "repeater_cliQuickGetRadio": "Get Radio", "repeater_cliQuickGetTx": "Get TX", diff --git a/lib/l10n/app_localizations.dart b/lib/l10n/app_localizations.dart index 517fcca..ff7b326 100644 --- a/lib/l10n/app_localizations.dart +++ b/lib/l10n/app_localizations.dart @@ -5126,6 +5126,24 @@ abstract class AppLocalizations { /// **'Error: {error}'** String repeater_cliCommandError(String error); + /// No description provided for @repeater_cliLateResponse. + /// + /// In en, this message translates to: + /// **'Late response to \"{command}\" (arrived {seconds}s after timeout)'** + String repeater_cliLateResponse(String command, String seconds); + + /// No description provided for @repeater_cliUnmatchedResponse. + /// + /// In en, this message translates to: + /// **'Unrequested response from repeater'** + String get repeater_cliUnmatchedResponse; + + /// No description provided for @repeater_lateResponseReceived. + /// + /// In en, this message translates to: + /// **'A late response arrived after the command timed out. See the CLI screen.'** + String get repeater_lateResponseReceived; + /// No description provided for @repeater_cliQuickGetName. /// /// In en, this message translates to: diff --git a/lib/l10n/app_localizations_bg.dart b/lib/l10n/app_localizations_bg.dart index b80f289..bc3b38a 100644 --- a/lib/l10n/app_localizations_bg.dart +++ b/lib/l10n/app_localizations_bg.dart @@ -2905,6 +2905,19 @@ class AppLocalizationsBg extends AppLocalizations { return 'Грешка: $error'; } + @override + String repeater_cliLateResponse(String command, String seconds) { + return 'Late response to \"$command\" (arrived ${seconds}s after timeout)'; + } + + @override + String get repeater_cliUnmatchedResponse => + 'Unrequested response from repeater'; + + @override + String get repeater_lateResponseReceived => + 'A late response arrived after the command timed out. See the CLI screen.'; + @override String get repeater_cliQuickGetName => 'Получи име'; diff --git a/lib/l10n/app_localizations_de.dart b/lib/l10n/app_localizations_de.dart index 221a7cc..4796cc0 100644 --- a/lib/l10n/app_localizations_de.dart +++ b/lib/l10n/app_localizations_de.dart @@ -2908,6 +2908,19 @@ class AppLocalizationsDe extends AppLocalizations { return 'Fehler: $error'; } + @override + String repeater_cliLateResponse(String command, String seconds) { + return 'Late response to \"$command\" (arrived ${seconds}s after timeout)'; + } + + @override + String get repeater_cliUnmatchedResponse => + 'Unrequested response from repeater'; + + @override + String get repeater_lateResponseReceived => + 'A late response arrived after the command timed out. See the CLI screen.'; + @override String get repeater_cliQuickGetName => 'Name erhalten'; diff --git a/lib/l10n/app_localizations_en.dart b/lib/l10n/app_localizations_en.dart index b483034..a2f753c 100644 --- a/lib/l10n/app_localizations_en.dart +++ b/lib/l10n/app_localizations_en.dart @@ -2849,6 +2849,19 @@ class AppLocalizationsEn extends AppLocalizations { return 'Error: $error'; } + @override + String repeater_cliLateResponse(String command, String seconds) { + return 'Late response to \"$command\" (arrived ${seconds}s after timeout)'; + } + + @override + String get repeater_cliUnmatchedResponse => + 'Unrequested response from repeater'; + + @override + String get repeater_lateResponseReceived => + 'A late response arrived after the command timed out. See the CLI screen.'; + @override String get repeater_cliQuickGetName => 'Get Name'; diff --git a/lib/l10n/app_localizations_es.dart b/lib/l10n/app_localizations_es.dart index 8b09b37..ada7780 100644 --- a/lib/l10n/app_localizations_es.dart +++ b/lib/l10n/app_localizations_es.dart @@ -2899,6 +2899,19 @@ class AppLocalizationsEs extends AppLocalizations { return 'Error: $error'; } + @override + String repeater_cliLateResponse(String command, String seconds) { + return 'Late response to \"$command\" (arrived ${seconds}s after timeout)'; + } + + @override + String get repeater_cliUnmatchedResponse => + 'Unrequested response from repeater'; + + @override + String get repeater_lateResponseReceived => + 'A late response arrived after the command timed out. See the CLI screen.'; + @override String get repeater_cliQuickGetName => 'Obtener Nombre'; diff --git a/lib/l10n/app_localizations_fr.dart b/lib/l10n/app_localizations_fr.dart index 5545ce0..48cc33d 100644 --- a/lib/l10n/app_localizations_fr.dart +++ b/lib/l10n/app_localizations_fr.dart @@ -2921,6 +2921,19 @@ class AppLocalizationsFr extends AppLocalizations { return 'Erreur : $error'; } + @override + String repeater_cliLateResponse(String command, String seconds) { + return 'Late response to \"$command\" (arrived ${seconds}s after timeout)'; + } + + @override + String get repeater_cliUnmatchedResponse => + 'Unrequested response from repeater'; + + @override + String get repeater_lateResponseReceived => + 'A late response arrived after the command timed out. See the CLI screen.'; + @override String get repeater_cliQuickGetName => 'Obtenir le nom'; diff --git a/lib/l10n/app_localizations_hu.dart b/lib/l10n/app_localizations_hu.dart index d3d4fff..c03f5cc 100644 --- a/lib/l10n/app_localizations_hu.dart +++ b/lib/l10n/app_localizations_hu.dart @@ -2912,6 +2912,19 @@ class AppLocalizationsHu extends AppLocalizations { return 'Hiba: $error'; } + @override + String repeater_cliLateResponse(String command, String seconds) { + return 'Late response to \"$command\" (arrived ${seconds}s after timeout)'; + } + + @override + String get repeater_cliUnmatchedResponse => + 'Unrequested response from repeater'; + + @override + String get repeater_lateResponseReceived => + 'A late response arrived after the command timed out. See the CLI screen.'; + @override String get repeater_cliQuickGetName => 'Kapcsold össze a nevet'; diff --git a/lib/l10n/app_localizations_it.dart b/lib/l10n/app_localizations_it.dart index 7ce690b..74f1361 100644 --- a/lib/l10n/app_localizations_it.dart +++ b/lib/l10n/app_localizations_it.dart @@ -2905,6 +2905,19 @@ class AppLocalizationsIt extends AppLocalizations { return 'Errore: $error'; } + @override + String repeater_cliLateResponse(String command, String seconds) { + return 'Late response to \"$command\" (arrived ${seconds}s after timeout)'; + } + + @override + String get repeater_cliUnmatchedResponse => + 'Unrequested response from repeater'; + + @override + String get repeater_lateResponseReceived => + 'A late response arrived after the command timed out. See the CLI screen.'; + @override String get repeater_cliQuickGetName => 'Ottieni Nome'; diff --git a/lib/l10n/app_localizations_ja.dart b/lib/l10n/app_localizations_ja.dart index e1f2f0b..736587e 100644 --- a/lib/l10n/app_localizations_ja.dart +++ b/lib/l10n/app_localizations_ja.dart @@ -2785,6 +2785,19 @@ class AppLocalizationsJa extends AppLocalizations { return 'エラー：$error'; } + @override + String repeater_cliLateResponse(String command, String seconds) { + return 'Late response to \"$command\" (arrived ${seconds}s after timeout)'; + } + + @override + String get repeater_cliUnmatchedResponse => + 'Unrequested response from repeater'; + + @override + String get repeater_lateResponseReceived => + 'A late response arrived after the command timed out. See the CLI screen.'; + @override String get repeater_cliQuickGetName => '名前を取得する'; diff --git a/lib/l10n/app_localizations_ko.dart b/lib/l10n/app_localizations_ko.dart index 8efc350..0892237 100644 --- a/lib/l10n/app_localizations_ko.dart +++ b/lib/l10n/app_localizations_ko.dart @@ -2783,6 +2783,19 @@ class AppLocalizationsKo extends AppLocalizations { return '오류: $error'; } + @override + String repeater_cliLateResponse(String command, String seconds) { + return 'Late response to \"$command\" (arrived ${seconds}s after timeout)'; + } + + @override + String get repeater_cliUnmatchedResponse => + 'Unrequested response from repeater'; + + @override + String get repeater_lateResponseReceived => + 'A late response arrived after the command timed out. See the CLI screen.'; + @override String get repeater_cliQuickGetName => '이름을 알려주세요'; diff --git a/lib/l10n/app_localizations_nl.dart b/lib/l10n/app_localizations_nl.dart index 20f2ef5..3e9e5bc 100644 --- a/lib/l10n/app_localizations_nl.dart +++ b/lib/l10n/app_localizations_nl.dart @@ -2885,6 +2885,19 @@ class AppLocalizationsNl extends AppLocalizations { return 'Fout: $error'; } + @override + String repeater_cliLateResponse(String command, String seconds) { + return 'Late response to \"$command\" (arrived ${seconds}s after timeout)'; + } + + @override + String get repeater_cliUnmatchedResponse => + 'Unrequested response from repeater'; + + @override + String get repeater_lateResponseReceived => + 'A late response arrived after the command timed out. See the CLI screen.'; + @override String get repeater_cliQuickGetName => 'Naam opvragen'; diff --git a/lib/l10n/app_localizations_pl.dart b/lib/l10n/app_localizations_pl.dart index 3587475..1eb1a6a 100644 --- a/lib/l10n/app_localizations_pl.dart +++ b/lib/l10n/app_localizations_pl.dart @@ -2913,6 +2913,19 @@ class AppLocalizationsPl extends AppLocalizations { return 'Błąd: $error'; } + @override + String repeater_cliLateResponse(String command, String seconds) { + return 'Late response to \"$command\" (arrived ${seconds}s after timeout)'; + } + + @override + String get repeater_cliUnmatchedResponse => + 'Unrequested response from repeater'; + + @override + String get repeater_lateResponseReceived => + 'A late response arrived after the command timed out. See the CLI screen.'; + @override String get repeater_cliQuickGetName => 'Pobierz nazwę'; diff --git a/lib/l10n/app_localizations_pt.dart b/lib/l10n/app_localizations_pt.dart index 6585573..ae4403e 100644 --- a/lib/l10n/app_localizations_pt.dart +++ b/lib/l10n/app_localizations_pt.dart @@ -2897,6 +2897,19 @@ class AppLocalizationsPt extends AppLocalizations { return 'Erro: $error'; } + @override + String repeater_cliLateResponse(String command, String seconds) { + return 'Late response to \"$command\" (arrived ${seconds}s after timeout)'; + } + + @override + String get repeater_cliUnmatchedResponse => + 'Unrequested response from repeater'; + + @override + String get repeater_lateResponseReceived => + 'A late response arrived after the command timed out. See the CLI screen.'; + @override String get repeater_cliQuickGetName => 'Obter Nome'; diff --git a/lib/l10n/app_localizations_ru.dart b/lib/l10n/app_localizations_ru.dart index fcdeea8..269db9b 100644 --- a/lib/l10n/app_localizations_ru.dart +++ b/lib/l10n/app_localizations_ru.dart @@ -2903,6 +2903,19 @@ class AppLocalizationsRu extends AppLocalizations { return 'Ошибка: $error'; } + @override + String repeater_cliLateResponse(String command, String seconds) { + return 'Late response to \"$command\" (arrived ${seconds}s after timeout)'; + } + + @override + String get repeater_cliUnmatchedResponse => + 'Unrequested response from repeater'; + + @override + String get repeater_lateResponseReceived => + 'A late response arrived after the command timed out. See the CLI screen.'; + @override String get repeater_cliQuickGetName => 'Получить имя'; diff --git a/lib/l10n/app_localizations_sk.dart b/lib/l10n/app_localizations_sk.dart index ce420bd..2acb38d 100644 --- a/lib/l10n/app_localizations_sk.dart +++ b/lib/l10n/app_localizations_sk.dart @@ -2884,6 +2884,19 @@ class AppLocalizationsSk extends AppLocalizations { return 'Chyba: $error'; } + @override + String repeater_cliLateResponse(String command, String seconds) { + return 'Late response to \"$command\" (arrived ${seconds}s after timeout)'; + } + + @override + String get repeater_cliUnmatchedResponse => + 'Unrequested response from repeater'; + + @override + String get repeater_lateResponseReceived => + 'A late response arrived after the command timed out. See the CLI screen.'; + @override String get repeater_cliQuickGetName => 'Zísť meno'; diff --git a/lib/l10n/app_localizations_sl.dart b/lib/l10n/app_localizations_sl.dart index 849e118..26e1fa6 100644 --- a/lib/l10n/app_localizations_sl.dart +++ b/lib/l10n/app_localizations_sl.dart @@ -2883,6 +2883,19 @@ class AppLocalizationsSl extends AppLocalizations { return 'Napaka: $error'; } + @override + String repeater_cliLateResponse(String command, String seconds) { + return 'Late response to \"$command\" (arrived ${seconds}s after timeout)'; + } + + @override + String get repeater_cliUnmatchedResponse => + 'Unrequested response from repeater'; + + @override + String get repeater_lateResponseReceived => + 'A late response arrived after the command timed out. See the CLI screen.'; + @override String get repeater_cliQuickGetName => 'Pridobi ime'; diff --git a/lib/l10n/app_localizations_sv.dart b/lib/l10n/app_localizations_sv.dart index d033982..4111819 100644 --- a/lib/l10n/app_localizations_sv.dart +++ b/lib/l10n/app_localizations_sv.dart @@ -2869,6 +2869,19 @@ class AppLocalizationsSv extends AppLocalizations { return 'Fel: $error'; } + @override + String repeater_cliLateResponse(String command, String seconds) { + return 'Late response to \"$command\" (arrived ${seconds}s after timeout)'; + } + + @override + String get repeater_cliUnmatchedResponse => + 'Unrequested response from repeater'; + + @override + String get repeater_lateResponseReceived => + 'A late response arrived after the command timed out. See the CLI screen.'; + @override String get repeater_cliQuickGetName => 'Hämta namn'; diff --git a/lib/l10n/app_localizations_uk.dart b/lib/l10n/app_localizations_uk.dart index 2e44f5f..997fe73 100644 --- a/lib/l10n/app_localizations_uk.dart +++ b/lib/l10n/app_localizations_uk.dart @@ -2901,6 +2901,19 @@ class AppLocalizationsUk extends AppLocalizations { return 'Помилка: $error'; } + @override + String repeater_cliLateResponse(String command, String seconds) { + return 'Late response to \"$command\" (arrived ${seconds}s after timeout)'; + } + + @override + String get repeater_cliUnmatchedResponse => + 'Unrequested response from repeater'; + + @override + String get repeater_lateResponseReceived => + 'A late response arrived after the command timed out. See the CLI screen.'; + @override String get repeater_cliQuickGetName => 'Отримати ім\'я'; diff --git a/lib/l10n/app_localizations_zh.dart b/lib/l10n/app_localizations_zh.dart index 578575c..f4eadf1 100644 --- a/lib/l10n/app_localizations_zh.dart +++ b/lib/l10n/app_localizations_zh.dart @@ -2737,6 +2737,19 @@ class AppLocalizationsZh extends AppLocalizations { return '错误：$error'; } + @override + String repeater_cliLateResponse(String command, String seconds) { + return 'Late response to \"$command\" (arrived ${seconds}s after timeout)'; + } + + @override + String get repeater_cliUnmatchedResponse => + 'Unrequested response from repeater'; + + @override + String get repeater_lateResponseReceived => + 'A late response arrived after the command timed out. See the CLI screen.'; + @override String get repeater_cliQuickGetName => '获取名称'; diff --git a/lib/screens/repeater_cli_screen.dart b/lib/screens/repeater_cli_screen.dart index 10e325a..82875ac 100644 --- a/lib/screens/repeater_cli_screen.dart +++ b/lib/screens/repeater_cli_screen.dart @@ -52,9 +52,31 @@ class _RepeaterCliScreenState extends State { super.initState(); final connector = Provider.of(context, listen: false); _commandService = RepeaterCommandService(connector); + _commandService!.onUnmatchedResponse = _handleUnmatchedResponse; _setupMessageListener(); } + /// A reply the command future never received, because its window had already + /// closed. It is still the repeater's real answer, so it goes into the + /// history instead of being dropped (#528). + void _handleUnmatchedResponse(UnmatchedRepeaterResponse unmatched) { + if (!mounted) return; + setState(() { + _commandHistory.add({ + 'type': 'late', + 'text': unmatched.response, + 'command': unmatched.command ?? '', + 'seconds': unmatched.sinceTimeout == null + ? '' + : (unmatched.sinceTimeout!.inMilliseconds / 1000).toStringAsFixed( + 1, + ), + 'timestamp': DateTime.now().toString(), + }); + }); + _scrollToBottom(); + } + @override void dispose() { _frameSubscription?.cancel(); @@ -103,11 +125,10 @@ class _RepeaterCliScreenState extends State { if (parsed == null) return; if (!_matchesRepeaterPrefix(parsed.senderPrefix)) return; - // Notify command service of response (for retry handling) + // The service routes this either to the waiting command's future, which + // _sendCommand appends to history, or to onUnmatchedResponse when the + // window has already closed. Both paths reach the transcript (#528). _commandService?.handleResponse(widget.repeater, parsed.text); - - // Note: The command service will handle the response via the Future - // We don't need to add it to history here anymore as _sendCommand will do it } bool _matchesRepeaterPrefix(Uint8List prefix) { @@ -184,7 +205,19 @@ class _RepeaterCliScreenState extends State { _historyIndex = -1; _commandFocusNode.requestFocus(); - // Auto-scroll to bottom + _scrollToBottom(); + } + + String _lateResponseLabel(Map entry) { + final command = entry['command'] ?? ''; + final seconds = entry['seconds'] ?? ''; + if (command.isEmpty || seconds.isEmpty) { + return context.l10n.repeater_cliUnmatchedResponse; + } + return context.l10n.repeater_cliLateResponse(command, seconds); + } + + void _scrollToBottom() { Future.delayed(const Duration(milliseconds: 100), () { if (_scrollController.hasClients) { _scrollController.animateTo( @@ -450,6 +483,25 @@ class _RepeaterCliScreenState extends State { itemBuilder: (context, index) { final entry = _commandHistory[index]; final isCommand = entry['type'] == 'command'; + final isLate = entry['type'] == 'late'; + final scheme = Theme.of(context).colorScheme; + + final Color badgeColor; + final Color badgeIconColor; + final IconData badgeIcon; + if (isCommand) { + badgeColor = scheme.primaryContainer; + badgeIconColor = scheme.onPrimaryContainer; + badgeIcon = Icons.chevron_right; + } else if (isLate) { + badgeColor = scheme.tertiaryContainer; + badgeIconColor = scheme.onTertiaryContainer; + badgeIcon = Icons.history; + } else { + badgeColor = scheme.secondaryContainer; + badgeIconColor = scheme.onSecondaryContainer; + badgeIcon = Icons.arrow_back; + } return Padding( padding: const EdgeInsets.only(bottom: 12), @@ -459,32 +511,34 @@ class _RepeaterCliScreenState extends State { Container( padding: const EdgeInsets.all(6), decoration: BoxDecoration( - color: isCommand - ? Theme.of(context).colorScheme.primaryContainer - : Theme.of(context).colorScheme.secondaryContainer, + color: badgeColor, borderRadius: BorderRadius.circular(4), ), - child: Icon( - isCommand ? Icons.chevron_right : Icons.arrow_back, - size: 16, - color: isCommand - ? Theme.of(context).colorScheme.onPrimaryContainer - : Theme.of(context).colorScheme.onSecondaryContainer, - ), + child: Icon(badgeIcon, size: 16, color: badgeIconColor), ), const SizedBox(width: 12), Expanded( child: Column( crossAxisAlignment: CrossAxisAlignment.start, children: [ + if (isLate) + Padding( + padding: const EdgeInsets.only(bottom: 2), + child: Text( + _lateResponseLabel(entry), + style: TextStyle( + fontSize: 11, + fontStyle: FontStyle.italic, + color: scheme.tertiary, + ), + ), + ), SelectableText( entry['text']!, style: TextStyle( fontFamily: 'monospace', fontSize: 13, - color: isCommand - ? Theme.of(context).colorScheme.primary - : Theme.of(context).colorScheme.onSurface, + color: isCommand ? scheme.primary : scheme.onSurface, ), ), ], diff --git a/lib/screens/repeater_settings_screen.dart b/lib/screens/repeater_settings_screen.dart index 8e5c892..1160daa 100644 --- a/lib/screens/repeater_settings_screen.dart +++ b/lib/screens/repeater_settings_screen.dart @@ -183,10 +183,26 @@ class _RepeaterSettingsScreenState extends State { super.initState(); final connector = Provider.of(context, listen: false); _commandService = RepeaterCommandService(connector); + _commandService!.onUnmatchedResponse = _handleUnmatchedResponse; _setupMessageListener(); _loadSettings(); } + /// A repeater reply whose command had already timed out. This screen has no + /// transcript to append it to, so it is applied like any other response and + /// the user is told it arrived late rather than the reply being dropped + /// (#528). + void _handleUnmatchedResponse(UnmatchedRepeaterResponse unmatched) { + if (!mounted) return; + if (unmatched.command != null) { + _handleGetResponse(unmatched.command!, unmatched.response); + } + showDismissibleSnackBar( + context, + content: Text(context.l10n.repeater_lateResponseReceived), + ); + } + @override void dispose() { _frameSubscription?.cancel(); diff --git a/lib/services/repeater_command_service.dart b/lib/services/repeater_command_service.dart index 61d22c8..be074f8 100644 --- a/lib/services/repeater_command_service.dart +++ b/lib/services/repeater_command_service.dart @@ -1,8 +1,40 @@ import 'dart:async'; +import 'package:flutter/foundation.dart'; import '../models/contact.dart'; import '../models/path_selection.dart'; import '../connector/meshcore_connector.dart'; import '../connector/meshcore_protocol.dart'; +import '../utils/app_logger.dart'; + +/// A repeater reply that arrived with no command waiting to receive it. +/// +/// [command] carries the original request when the reply could be traced back +/// to one that already timed out, and is null when the reply cannot be +/// attributed to anything this service sent. +class UnmatchedRepeaterResponse { + final String repeaterKeyHex; + final String response; + final String? command; + final Duration? sinceTimeout; + + const UnmatchedRepeaterResponse({ + required this.repeaterKeyHex, + required this.response, + this.command, + this.sinceTimeout, + }); + + /// True when this answers a request that timed out rather than arriving + /// unsolicited. + bool get isLateReply => command != null; +} + +class _ExpiredCommand { + final String command; + final DateTime expiredAt; + + const _ExpiredCommand({required this.command, required this.expiredAt}); +} class RepeaterCommandService { final MeshCoreConnector _connector; @@ -10,10 +42,20 @@ class RepeaterCommandService { final Map _commandTimeouts = {}; final Map _commandPrefixes = {}; final Map _pendingByPrefix = {}; + final Map _expiredCommands = {}; int _prefixCounter = 0; static const int maxRetries = 5; + /// How long a timed-out command stays remembered so a reply arriving after + /// its window can still be presented with the request it answers. + static const Duration lateReplyRetention = Duration(minutes: 2); + + /// Invoked when a reply cannot be handed to a waiting command. Consumers + /// must surface this to the user: the reply is a real answer from the + /// repeater and dropping it silently loses it for good (#528). + void Function(UnmatchedRepeaterResponse)? onUnmatchedResponse; + RepeaterCommandService(this._connector); /// Send a CLI command to a repeater with automatic retries @@ -27,6 +69,7 @@ class RepeaterCommandService { }) async { final attemptCount = retries < 1 ? 1 : retries; final selection = await _connector.preparePathForContactSend(repeater); + final attemptPrefixes = []; for (int attempt = 0; attempt < attemptCount; attempt++) { onAttempt?.call(attempt + 1); @@ -36,7 +79,13 @@ class RepeaterCommandService { command, selection, attempt, + attemptPrefixes, ); + // The caller has its answer, so a straggler from an earlier attempt of + // this same command is noise rather than a lost response. + for (final prefix in attemptPrefixes) { + _expiredCommands.remove(prefix); + } onResponse?.call(response); return response; } catch (e) { @@ -52,6 +101,7 @@ class RepeaterCommandService { String command, PathSelection selection, int attempt, + List attemptPrefixes, ) async { final repeaterKey = repeater.publicKeyHex; final prefix = _nextPrefixToken(); @@ -60,6 +110,7 @@ class RepeaterCommandService { _pendingCommands[commandId] = completer; _commandPrefixes[commandId] = prefix; _pendingByPrefix[prefix] = commandId; + attemptPrefixes.add(prefix); try { final framedCommand = '$prefix$command'; @@ -93,6 +144,12 @@ class RepeaterCommandService { () { final completer = _pendingCommands[commandId]; if (completer != null && !completer.isCompleted) { + // Remember what this prefix asked so a reply arriving after the + // window can still reach the user with its question attached. + _expiredCommands[prefix] = _ExpiredCommand( + command: command, + expiredAt: DateTime.now(), + ); completer.completeError( 'Command timeout after $timeoutSeconds seconds', ); @@ -114,29 +171,89 @@ class RepeaterCommandService { /// Call this when a text message response is received from a repeater void handleResponse(Contact repeater, String responseText) { - // Find pending command for this repeater and complete it final repeaterKey = repeater.publicKeyHex; + _pruneExpiredCommands(); - String? commandId; + String? prefix; String responsePayload = responseText; if (responseText.length >= 3 && responseText[2] == '|') { - final prefix = responseText.substring(0, 3); - commandId = _pendingByPrefix[prefix]; + prefix = responseText.substring(0, 3); responsePayload = responseText.substring(3).trimLeft(); } - commandId ??= _pendingCommands.keys.firstWhere( - (id) => id.startsWith(repeaterKey), - orElse: () => '', - ); + final matchedId = prefix != null ? _pendingByPrefix[prefix] : null; + final commandId = + matchedId ?? + _pendingCommands.keys.firstWhere( + (id) => id.startsWith(repeaterKey), + orElse: () => '', + ); + + if (commandId.isNotEmpty) { + final completer = _pendingCommands[commandId]; + if (completer != null && !completer.isCompleted) { + completer.complete(responsePayload); + _cleanup(commandId); + return; + } + } + + // Nothing is waiting for this reply. It is still a real answer from the + // repeater, so it gets surfaced and logged rather than dropped (#528). + _surfaceUnmatchedResponse(repeaterKey, prefix, responsePayload); + } - if (commandId.isEmpty) return; + void _surfaceUnmatchedResponse( + String repeaterKey, + String? prefix, + String responsePayload, + ) { + final expired = prefix != null ? _expiredCommands.remove(prefix) : null; + final sinceTimeout = expired == null + ? null + : DateTime.now().difference(expired.expiredAt); - final completer = _pendingCommands[commandId]; - if (completer != null && !completer.isCompleted) { - completer.complete(responsePayload); - _cleanup(commandId); + if (expired != null) { + appLogger.warn( + 'Late reply to "${expired.command}" from $repeaterKey arrived ' + '${sinceTimeout!.inMilliseconds}ms after its window closed', + tag: 'RepeaterCommand', + ); + } else { + appLogger.warn( + 'Reply from $repeaterKey matched no pending or recently expired ' + 'command (prefix: ${prefix ?? 'none'})', + tag: 'RepeaterCommand', + ); } + + onUnmatchedResponse?.call( + UnmatchedRepeaterResponse( + repeaterKeyHex: repeaterKey, + response: responsePayload, + command: expired?.command, + sinceTimeout: sinceTimeout, + ), + ); + } + + /// Records a timed-out command so a reply arriving later can still be + /// attributed to it. Exposed because a test cannot drive a real + /// send-then-time-out cycle without a connected transport. + @visibleForTesting + void recordExpiredCommandForTest(String prefix, String command) { + _expiredCommands[prefix] = _ExpiredCommand( + command: command, + expiredAt: DateTime.now(), + ); + } + + void _pruneExpiredCommands() { + if (_expiredCommands.isEmpty) return; + final cutoff = DateTime.now().subtract(lateReplyRetention); + _expiredCommands.removeWhere( + (_, entry) => entry.expiredAt.isBefore(cutoff), + ); } void _cleanup(String commandId) { @@ -157,6 +274,8 @@ class RepeaterCommandService { _pendingCommands.clear(); _commandPrefixes.clear(); _pendingByPrefix.clear(); + _expiredCommands.clear(); + onUnmatchedResponse = null; } String _nextPrefixToken() { diff --git a/test/services/repeater_command_service_test.dart b/test/services/repeater_command_service_test.dart new file mode 100644 index 0000000..df75aa8 --- /dev/null +++ b/test/services/repeater_command_service_test.dart @@ -0,0 +1,117 @@ +// #528 (epic #473): a repeater CLI reply that arrives with no command waiting +// for it used to hit `if (commandId.isEmpty) return;` and vanish. No log, no +// UI, nothing. Since a command's window can close before the app has even +// finished fetching the message (MSG_WAITING then SYNC_NEXT_MESSAGE), that +// made "the command ran but the response was never reported" the normal +// outcome rather than an edge case. +// +// These tests pin the contract that no reply is ever dropped silently. + +import 'dart:typed_data'; + +import 'package:flutter_test/flutter_test.dart'; +import 'package:meshcore_open/connector/meshcore_connector.dart'; +import 'package:meshcore_open/models/contact.dart'; +import 'package:meshcore_open/services/repeater_command_service.dart'; +import 'package:meshcore_open/storage/prefs_manager.dart'; +import 'package:shared_preferences/shared_preferences.dart'; + +Contact _repeater() => Contact( + publicKey: Uint8List.fromList(List.generate(32, (i) => i)), + name: 'Test Repeater', + type: 2, + pathLength: 0, + path: Uint8List(0), + lastSeen: DateTime.fromMillisecondsSinceEpoch(0), +); + +void main() { + TestWidgetsFlutterBinding.ensureInitialized(); + + late RepeaterCommandService service; + late Contact repeater; + late List surfaced; + + setUp(() async { + SharedPreferences.setMockInitialValues({}); + PrefsManager.reset(); + await PrefsManager.initialize(); + + service = RepeaterCommandService(MeshCoreConnector()); + repeater = _repeater(); + surfaced = []; + service.onUnmatchedResponse = surfaced.add; + }); + + tearDown(() => service.dispose()); + + test('a reply with no pending command is surfaced, not discarded', () { + service.handleResponse(repeater, 'Version: 1.2.3'); + + expect(surfaced, hasLength(1)); + expect(surfaced.single.response, 'Version: 1.2.3'); + expect(surfaced.single.repeaterKeyHex, repeater.publicKeyHex); + }); + + test('a reply for a command that already timed out names that command', () { + service.recordExpiredCommandForTest('A3|', 'ver'); + + service.handleResponse(repeater, 'A3|Version: 1.2.3'); + + expect(surfaced, hasLength(1)); + final reply = surfaced.single; + expect(reply.isLateReply, isTrue); + expect(reply.command, 'ver'); + expect(reply.response, 'Version: 1.2.3', reason: 'prefix must be stripped'); + expect(reply.sinceTimeout, isNotNull); + }); + + test('an unattributable reply is still surfaced, just without a command', () { + service.handleResponse(repeater, 'ZZ|unsolicited chatter'); + + expect(surfaced, hasLength(1)); + expect(surfaced.single.isLateReply, isFalse); + expect(surfaced.single.command, isNull); + expect(surfaced.single.response, 'unsolicited chatter'); + }); + + test('an expired command is only consumed once', () { + service.recordExpiredCommandForTest('A3|', 'ver'); + + service.handleResponse(repeater, 'A3|first'); + service.handleResponse(repeater, 'A3|second'); + + expect(surfaced, hasLength(2)); + expect(surfaced[0].command, 'ver'); + expect( + surfaced[1].command, + isNull, + reason: 'the record is consumed by the first reply that claims it', + ); + }); + + test('a reply is never swallowed when no callback is wired', () { + service.onUnmatchedResponse = null; + + // The contract is that this cannot throw and cannot hang. The logging path + // still runs; the absence of a listener must not resurrect the silent drop + // as an unhandled error. + expect( + () => service.handleResponse(repeater, 'orphan reply'), + returnsNormally, + ); + }); + + test('dispose clears the late-reply records', () { + service.recordExpiredCommandForTest('A3|', 'ver'); + service.dispose(); + + // Re-arm a listener on the disposed service and confirm the stale record is + // gone, so a reply after disposal cannot be mis-attributed to it. + final seen = []; + service.onUnmatchedResponse = seen.add; + service.handleResponse(repeater, 'A3|Version: 1.2.3'); + + expect(seen.single.command, isNull); + }); +}