From dab208877f3ba4f74ff5bbf312c07986ec97a256 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Elias=20M=C3=BCller?= Date: Sun, 5 Jul 2026 23:43:58 +0200 Subject: [PATCH] improved reliability and error handling for background notification actions by migrating to a top-level entry point for stable AOT callback resolution; implemented failure notifications to preserve undelivered reply text and surface technical errors; resolved a race condition that could resurrect dismissed notifications during silent re-renders; updated the Android manifest with the required broadcast receiver for notification actions; and refactored internal OCS methods to return detailed success/failure records for improved logging and field debugging. --- android/app/src/main/AndroidManifest.xml | 8 + lib/notification/notification_service.dart | 5 +- lib/push/push_actions.dart | 170 ++++++++++++++++----- lib/push/push_renderer.dart | 48 +++++- test/push/push_reply_action_test.dart | 20 +++ 5 files changed, 214 insertions(+), 37 deletions(-) diff --git a/android/app/src/main/AndroidManifest.xml b/android/app/src/main/AndroidManifest.xml index 15201b6..4a90918 100644 --- a/android/app/src/main/AndroidManifest.xml +++ b/android/app/src/main/AndroidManifest.xml @@ -59,6 +59,14 @@ android:name="com.google.firebase.messaging.default_notification_color" android:resource="@color/notification_accent" /> + + + pushActionBackgroundHandler(NotificationResponse response) => + PushActions.handleBackgroundResponse(response); + +/// Logs to the developer log AND to stdout: `dart:developer` messages are +/// invisible in release logcat, but field debugging of the notification +/// action isolates needs `adb logcat -s flutter` to show what happened. +void _plog(String message) { + log(message); + debugPrint('PushActions: $message'); +} + /// Handles Talk notification actions (inline reply, mark-as-read). Runs in the /// background isolate spawned by flutter_local_notifications, so it may not /// share any app state — it reads credentials straight from secure storage via /// the [AccountData] singleton after awaiting population. +/// +/// The class-level `vm:entry-point` pragma is REQUIRED in addition to the one +/// on [handleBackgroundResponse]: the callback is resolved via +/// `PluginUtilities.getCallbackFromHandle`, and in AOT builds resolving a +/// static method needs its enclosing class to be an entry point too — +/// otherwise the lookup fails with "To access ... PushActions from native +/// code, it must be annotated" and no action ever reaches Dart. +@pragma('vm:entry-point') class PushActions { /// Background entry point for notification actions. Must be a top-level or /// static function annotated with `vm:entry-point` so AOT keeps it alive. @@ -36,38 +61,101 @@ class PushActions { // runs forever. DartPluginRegistrant.ensureInitialized(); + _plog( + 'action=${response.actionId} payload=${response.payload} ' + 'hasInput=${response.input?.isNotEmpty ?? false}', + ); final chatToken = _chatTokenFrom(response.payload); if (chatToken == null) return; switch (response.actionId) { case kTalkReplyActionId: final text = response.input?.trim(); - final sent = text != null && text.isNotEmpty - ? await sendReply(chatToken, text) - : false; - if (sent) { + final result = text == null || text.isEmpty + ? ( + ok: false, + detail: 'Keine Texteingabe empfangen (RemoteInput leer).', + ) + : await sendReply(chatToken, text); + // Local cleanup FIRST, mark-read AFTER: markRead makes the server + // dismiss its notifications and emit delete-pushes. If those arrive + // in the FCM isolate while the thread history still exists here, the + // delete handler re-renders the thread and resurrects the just- + // cancelled notification (with Android re-attaching the pending + // inline reply on top). + await finishReply(chatToken: chatToken, sent: result.ok); + if (result.ok) { + _cleanupNidEntry(response); // The user has evidently seen the chat — set the read marker like - // the mark-read action does. + // the mark-read action does. Best effort: a failure here must not + // fail the already-delivered reply. await markRead(chatToken); } - await finishReply(chatToken: chatToken, sent: sent); - _cleanupNidEntry(response); + if (!result.ok) { + // Never swallow the typed message: surface the failure (and the + // undelivered text) as its own notification. The nid mapping stays + // alive — the thread notification is still in the tray. + await _showActionError( + chatToken: chatToken, + title: 'Antwort nicht gesendet', + body: actionFailureBody(lostText: text, detail: result.detail), + ); + } break; case kTalkMarkReadActionId: - await markRead(chatToken); + // Optimistic like the in-app read-marker: clean up locally first so + // the server's delete-pushes (triggered by markRead) can never race a + // still-present thread history into a resurrected notification. await _cleanupChat(chatToken); _cleanupNidEntry(response); + final result = await markRead(chatToken); + if (!result.ok) { + await _showActionError( + chatToken: chatToken, + title: 'Als gelesen markieren fehlgeschlagen', + body: actionFailureBody(detail: result.detail), + ); + } break; default: break; } } - /// Ends the reply interaction: Android keeps the RemoteInput spinner alive - /// until the notification is UPDATED or REMOVED. Success removes the whole - /// notification (history cleared + cancel — the chat is read); failure - /// re-renders the unchanged thread silently so the spinner stops (the error - /// itself is only loggable on notification level). Injectable seams so - /// tests can observe the flow without platform channels. + /// Body text of an action-failure notification: the undelivered reply (if + /// any) followed by the technical reason. + static String actionFailureBody({String? lostText, required String detail}) { + return [ + if (lostText != null && lostText.isNotEmpty) 'Deine Nachricht: „$lostText“', + 'Grund: $detail', + ].join('\n'); + } + + static Future _showActionError({ + required String chatToken, + required String title, + required String body, + }) async { + try { + await PushRenderer().renderTalkActionError( + chatToken: chatToken, + title: title, + body: body, + ); + } on Object catch (e) { + _plog('Push action error notification failed: $e'); + } + } + + /// Ends the reply interaction. The card itself is already gone — the + /// action's native `cancelNotification` removed it the moment the reply + /// fired (a Dart-side cancel instead does NOT work everywhere: MIUI/HyperOS + /// ignores app cancels while an inline reply is pending). What remains: + /// success clears the stacked history so the next push starts fresh (plus a + /// redundant defensive cancel); failure re-renders silently, which the + /// renderer's active-probe turns into a no-op when the card is really gone + /// — the failure surface is the error card posted by the caller. + /// Injectable seams so tests can observe the flow without platform + /// channels. static Future finishReply({ required String chatToken, required bool sent, @@ -100,21 +188,31 @@ class PushActions { /// Sends the inline reply. Success = any 2xx (the Talk chat POST answers /// 201 Created). Path/body match the app's working send path /// (`SendMessage`: `v1/chat/{token}`, form field `message`). - static Future sendReply(String chatToken, String message) => _ocsPost( - 'apps/spreed/api/v1/chat/$chatToken', - body: {'message': message}, - ); + static Future<({bool ok, String detail})> sendReply( + String chatToken, + String message, + ) => _ocsPost('apps/spreed/api/v1/chat/$chatToken', body: {'message': message}); - static Future markRead(String chatToken) => + static Future<({bool ok, String detail})> markRead(String chatToken) => _ocsPost('apps/spreed/api/v1/chat/$chatToken/read'); - static Future _ocsPost(String path, {Map? body}) async { + static Future<({bool ok, String detail})> _ocsPost( + String path, { + Map? body, + }) async { try { // Bounded: a hanging population (e.g. keystore issue) must fail the // action instead of leaving the notification spinner running forever. - await AccountData().waitForPopulation().timeout( + final populated = await AccountData().waitForPopulation().timeout( const Duration(seconds: 10), ); + if (!populated) { + _plog('Push action $path aborted: credentials unreadable in isolate'); + return ( + ok: false, + detail: 'Zugangsdaten im Hintergrund-Prozess nicht lesbar.', + ); + } final response = await http.post( NextcloudOcs.uri(path), headers: NextcloudOcs.headers(), @@ -122,18 +220,22 @@ class PushActions { ); final ok = response.statusCode >= 200 && response.statusCode < 300; if (ok) { - log('Push action $path -> HTTP ${response.statusCode}'); - } else { - final preview = response.body.replaceAll(RegExp(r'\s+'), ' ').trim(); - log( - 'Push action $path -> HTTP ${response.statusCode} ' - 'body=${preview.length > 300 ? '${preview.substring(0, 300)}…' : preview}', - ); + _plog('Push action $path -> HTTP ${response.statusCode}'); + return (ok: true, detail: 'HTTP ${response.statusCode}'); } - return ok; + final preview = response.body.replaceAll(RegExp(r'\s+'), ' ').trim(); + final trimmed = preview.length > 200 + ? '${preview.substring(0, 200)}…' + : preview; + _plog('Push action $path -> HTTP ${response.statusCode} body=$trimmed'); + return ( + ok: false, + detail: + 'HTTP ${response.statusCode}${trimmed.isEmpty ? '' : ' – $trimmed'}', + ); } on Object catch (e) { - log('Push action $path failed: $e'); - return false; + _plog('Push action $path failed: $e'); + return (ok: false, detail: e.toString()); } } @@ -143,7 +245,7 @@ class PushActions { try { await ChatThreadStore().clearChat(chatToken); } on Object catch (e) { - log('Push action thread cleanup failed: $e'); + _plog('Push action thread cleanup failed: $e'); } await _cancelChatNotification(chatToken); } @@ -155,7 +257,7 @@ class PushActions { tag: chatNotificationTag(chatToken), ); } on Object catch (e) { - log('Push action cancel failed: $e'); + _plog('Push action cancel failed: $e'); } } @@ -167,7 +269,7 @@ class PushActions { .delete(nid) .then( (_) {}, - onError: (Object e) => log('Push action nid cleanup failed: $e'), + onError: (Object e) => _plog('Push action nid cleanup failed: $e'), ), ); } diff --git a/lib/push/push_renderer.dart b/lib/push/push_renderer.dart index 9aadb0a..0f8d307 100644 --- a/lib/push/push_renderer.dart +++ b/lib/push/push_renderer.dart @@ -2,6 +2,7 @@ import 'dart:async'; import 'dart:convert'; import 'dart:developer'; +import 'package:flutter/foundation.dart'; import 'package:flutter_local_notifications/flutter_local_notifications.dart'; import '../notification/notification_service.dart'; @@ -146,6 +147,17 @@ class PushRenderer { bool alert = true, }) async { if (messages.isEmpty) return; + // A silent render only ever UPDATES an existing card (delete-push shrunk + // the thread, late avatar arrived, failed reply needs its spinner + // stopped). If the card is verifiably gone meanwhile (reply/mark-read + // cleanup cancelled it, or the user swiped it away), re-posting would + // resurrect it — and Android would re-attach a pending inline reply on + // top. Probe failure (null) still renders: stopping a possible reply + // spinner outweighs a rare resurrection. + if (!alert && await _isChatNotificationActive(chatToken) == false) { + debugPrint('PushRenderer: skip silent re-render, card gone ($chatToken)'); + return; + } final tag = chatNotificationTag(chatToken); final id = stableChatNotificationId(chatToken); final latest = messages.last; @@ -261,12 +273,18 @@ class PushRenderer { ); } + // Both actions keep cancelNotification: true (the default): the plugin's + // Java receiver then removes the card NATIVELY the moment the action fires. + // The reply action must not rely on our Dart-side cancel instead — MIUI/ + // HyperOS ignores an app-issued cancel while an inline reply is pending, so + // the card would stay behind showing the reply as an attached "Ich" row. + // If sending subsequently fails, the error card (with the typed text) + // replaces the lost thread card. static const _talkActions = [ AndroidNotificationAction( kTalkReplyActionId, 'Antworten', showsUserInterface: false, - cancelNotification: false, inputs: [AndroidNotificationActionInput(label: 'Nachricht')], ), AndroidNotificationAction( @@ -276,6 +294,34 @@ class PushRenderer { ), ]; + /// Renders a failure card for a Talk notification action (reply/mark-read). + /// Separate tag per chat, so it neither replaces the thread notification + /// nor stacks across repeated failures. Carries no payload — tapping it + /// just opens the app. + Future renderTalkActionError({ + required String chatToken, + required String title, + required String body, + }) async { + await _plugin.show( + id: stableChatNotificationId(chatToken), + title: title, + body: body, + notificationDetails: NotificationDetails( + android: AndroidNotificationDetails( + talkChannelId, + talkChannelName, + importance: Importance.high, + priority: Priority.high, + color: _accentColor, + tag: 'talk_error_$chatToken', + styleInformation: BigTextStyleInformation(body), + ), + iOS: const DarwinNotificationDetails(), + ), + ); + } + Future _renderGeneric(PushSubject subject) async { final nid = subject.nid ?? _fallbackId(subject.subject); const androidDetails = AndroidNotificationDetails( diff --git a/test/push/push_reply_action_test.dart b/test/push/push_reply_action_test.dart index ae2f9e3..0c5dba1 100644 --- a/test/push/push_reply_action_test.dart +++ b/test/push/push_reply_action_test.dart @@ -3,6 +3,26 @@ import 'package:marianum_mobile/push/chat_thread_store.dart'; import 'package:marianum_mobile/push/push_actions.dart'; void main() { + group('PushActions.actionFailureBody', () { + test('includes the undelivered text and the reason', () { + expect( + PushActions.actionFailureBody(lostText: 'Hallo du', detail: 'HTTP 401'), + 'Deine Nachricht: „Hallo du“\nGrund: HTTP 401', + ); + }); + + test('omits the text line when there is none', () { + expect( + PushActions.actionFailureBody(detail: 'HTTP 404'), + 'Grund: HTTP 404', + ); + expect( + PushActions.actionFailureBody(lostText: '', detail: 'HTTP 404'), + 'Grund: HTTP 404', + ); + }); + }); + group('PushActions.finishReply', () { const token = 'chat1'; final thread = [