fix(app,backend): BigText chat-answer notifications + chat deep-link - #13065
fix(app,backend): BigText chat-answer notifications + chat deep-link#13065apoorvdarshan wants to merge 12 commits into
Conversation
There was a problem hiding this comment.
3 issues found and verified against the latest diff
Confidence score: 2/5
app/lib/services/notifications/chat_answer_notification_handler.dart: chat-answer notifications are not dispatched by the FCM background callback when Android is backgrounded or terminated, so users may not receive chat-answer notifications; update the background path to handle this data.app/lib/services/notifications/notification_service_fcm.dart:getInitialMessage()may resolve before the navigator exists, causingnavigate_toto be dropped and opening the app on the wrong screen; defer initial-message navigation until the app is ready.backend/tests/unit/test_chat_answer_client_displayed_notification.py: the test cannot be imported becausetesting.import_isolationis missing from the repository, so this coverage cannot run; add the required helper or correct the test import.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/lib/services/notifications/chat_answer_notification_handler.dart">
<violation number="1" location="app/lib/services/notifications/chat_answer_notification_handler.dart:11">
P1: When an Android chat answer arrives while the app is backgrounded or terminated, this handler is never invoked because the FCM background callback does not dispatch chat-answer data. Data-only pushes do not create a system notification, so the answer is lost; invoke and await this handler from `_firebaseMessagingBackgroundHandler` as well.</violation>
</file>
<file name="app/lib/services/notifications/notification_service_fcm.dart">
<violation number="1" location="app/lib/services/notifications/notification_service_fcm.dart:280">
P1: When a terminated app is opened from an FCM notification, `getInitialMessage()` can complete during `_init`, before `runApp` creates the navigator. The null-aware push then drops `navigate_to`, so the app opens without the chat; retain the route and replay it after the navigator is mounted.</violation>
</file>
<file name="backend/tests/unit/test_chat_answer_client_displayed_notification.py">
<violation number="1" location="backend/tests/unit/test_chat_answer_client_displayed_notification.py:8">
P2: The import `from testing.import_isolation import load_module_fresh, stub_modules` fails because no `testing/import_isolation.py` exists in this repo (only this test references it). The module can't be imported, so the whole test file errors at collection time. Add the helper module or switch to `unittest.mock.patch`/`patch.object` per repo conventions.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| @@ -0,0 +1,78 @@ | |||
| import 'package:awesome_notifications/awesome_notifications.dart'; | |||
There was a problem hiding this comment.
P1: When an Android chat answer arrives while the app is backgrounded or terminated, this handler is never invoked because the FCM background callback does not dispatch chat-answer data. Data-only pushes do not create a system notification, so the answer is lost; invoke and await this handler from _firebaseMessagingBackgroundHandler as well.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/lib/services/notifications/chat_answer_notification_handler.dart, line 11:
<comment>When an Android chat answer arrives while the app is backgrounded or terminated, this handler is never invoked because the FCM background callback does not dispatch chat-answer data. Data-only pushes do not create a system notification, so the answer is lost; invoke and await this handler from `_firebaseMessagingBackgroundHandler` as well.</comment>
<file context>
@@ -0,0 +1,78 @@
+/// answers are therefore delivered as data-oriented pushes and rendered here
+/// so the shade shows more of the answer and the tap payload includes
+/// `navigate_to` for deep-linking into the matching chat.
+class ChatAnswerNotificationHandler {
+ static final _awesomeNotifications = AwesomeNotifications();
+
</file context>
| } | ||
|
|
||
| FirebaseMessaging.onMessageOpenedApp.listen(handleNotificationTap); | ||
| FirebaseMessaging.instance.getInitialMessage().then(handleNotificationTap); |
There was a problem hiding this comment.
P1: When a terminated app is opened from an FCM notification, getInitialMessage() can complete during _init, before runApp creates the navigator. The null-aware push then drops navigate_to, so the app opens without the chat; retain the route and replay it after the navigator is mounted.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/lib/services/notifications/notification_service_fcm.dart, line 280:
<comment>When a terminated app is opened from an FCM notification, `getInitialMessage()` can complete during `_init`, before `runApp` creates the navigator. The null-aware push then drops `navigate_to`, so the app opens without the chat; retain the route and replay it after the navigator is mounted.</comment>
<file context>
@@ -235,28 +230,54 @@ class _FCMNotificationService implements NotificationInterface {
+ }
+
+ FirebaseMessaging.onMessageOpenedApp.listen(handleNotificationTap);
+ FirebaseMessaging.instance.getInitialMessage().then(handleNotificationTap);
}
</file context>
| from types import ModuleType, SimpleNamespace | ||
| from typing import Any, Iterator | ||
|
|
||
| from testing.import_isolation import load_module_fresh, stub_modules |
There was a problem hiding this comment.
P2: The import from testing.import_isolation import load_module_fresh, stub_modules fails because no testing/import_isolation.py exists in this repo (only this test references it). The module can't be imported, so the whole test file errors at collection time. Add the helper module or switch to unittest.mock.patch/patch.object per repo conventions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/tests/unit/test_chat_answer_client_displayed_notification.py, line 8:
<comment>The import `from testing.import_isolation import load_module_fresh, stub_modules` fails because no `testing/import_isolation.py` exists in this repo (only this test references it). The module can't be imported, so the whole test file errors at collection time. Add the helper module or switch to `unittest.mock.patch`/`patch.object` per repo conventions.</comment>
<file context>
@@ -0,0 +1,153 @@
+from types import ModuleType, SimpleNamespace
+from typing import Any, Iterator
+
+from testing.import_isolation import load_module_fresh, stub_modules
+
+BACKEND_DIR = Path(__file__).resolve().parents[2]
</file context>
Git-on-my-level
left a comment
There was a problem hiding this comment.
Thanks @apoorvdarshan — this targets a real, triaged pain point (#4375), and the bones of the approach are right: chat answers delivered as data-oriented pushes so the client renders BigText locally, with a navigate_to payload for deep-linking. The backend test coverage for the new message shape is solid. The rework asked for below is mostly about integrating with where main is today.
What looks good
- backend/utils/chat_answer_notifications.py: the message shape is careful — notification=None with data on Android, an explicit APNS alert for iOS, and the same HTTPS-only guard for webpush fcm_options.link that utils/notifications.py uses; invalid-token pruning after send_each matches the existing pattern.
- app/lib/services/notifications/chat_answer_notification_handler.dart: isChatAnswerData() is a clean predicate and all four cases are covered in chat_answer_notification_handler_test.dart.
- The foreground path in notification_service_fcm.dart keeps the ServerMessage stream emission before the BigText branch, so in-app consumers still receive the message.
Requested changes
- Rebase onto current main — the big one. The merge base here is from mid-May, and the exact hunks this PR touches have evolved since:
- backend/utils/chat.py on main now builds the payload via _chat_message_notification(...) and has send_chat_message_notification_async(...) as the boundary for the streaming path. The PR keeps a synchronous FCM send inside the SSE generator (send_chat_message_notification in the done branch of process_message); main moved away from that deliberately — a blocking send stalls the stream. Please keep the async boundary when rebasing.
- app/lib/services/notifications/notification_service_fcm.dart on main already routes taps through NotificationUtil.navigateToFromFcmData + NotificationUtil.handleNavigateTo (#5126), which polls until the navigator exists before pushing. The new inline handleNotificationTap uses globalNavigatorKey.currentState?.pushReplacement, so on a cold start it silently drops navigate_to when the navigator is not mounted yet — the exact race #5126 fixed. Reusing NotificationUtil would delete that whole block and inherit the robustness.
- Please don't override the FCM background handler by deferred re-registration. The Future(() { FirebaseMessaging.onBackgroundMessage(omiFirebaseMessagingBackgroundHandler); }) in initialize() is a timing-dependent last-writer-wins against main.dart's registration. Upstream, the cleaner fix is to add the chat-answer branch directly to _firebaseMessagingBackgroundHandler in main.dart — it currently handles action-item/merge/important-conversation but not chat answers, which is the actual gap. fcm_background_handler.dart is a near-copy of that handler and already drifts from it (drops NotificationChannelStrings.loadAppLocale() and the shared _ensureFirebaseApp() bootstrap), and a second copy will keep drifting. The fork-compatibility rationale in its doc comment makes sense for a fork, but upstream should carry the change in the one handler.
- Backend: prefer extending the shared sender over a parallel module. chat_answer_notifications.py duplicates IOS_BUNDLE_ID, PERMANENT_FAILURE_CODES, the tag generator, and the send/prune loop from utils/notifications.py. A client-displayed (data-only) mode on utils/notifications.py — an is_data_only/client-rendered flag threaded through _build_message — would keep one source of truth for the APNS topic, failure codes, and token pruning. If maintainers prefer a separate module that is their call; flagging the duplication.
- Smaller notes:
- Dropping the FCM notification payload removes Android's system-tray fallback: if the background handler cannot run (Doze, OEM battery killers, first launch after update before the handler is registered), the answer is silently dropped where it used to at least appear collapsed. Arguably the right trade for BigText + deep-link, but it is a product call.
- wakeUpScreen: !isAppInForeground will light the screen for every backgrounded chat answer; that is usually reserved for time-critical notifications.
- backend/tests/unit/test_chat_answer_client_displayed_notification.py imports testing.import_isolation, which exists on main but not on this branch's merge base — one more reason the rebase matters; the usage itself matches the sanctioned API.
- chat.py also deletes a dozen unrelated comments; dropping that churn would make the real change easier to review.
Leaving for human maintainer review after the rebase: the data-only delivery trade-off (no system-tray fallback when the client handler cannot run) and the wakeUpScreen behavior are product decisions on the notification surface.
by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.
|
Superseded by #13173 — clean reimplementation on current Closing this PR as superseded; please review #13173 instead. |
|
Hey @apoorvdarshan 👋 Thank you so much for taking the time to contribute to Omi! We truly appreciate you putting in the effort to submit this pull request. After careful review, we've decided not to merge this particular PR. Please don't take this personally — we genuinely try to merge as many contributions as possible, but sometimes we have to make tough calls based on:
Before your next PR, please skim:
If this was declined for direction or taste, maintainers should cite an invariant ID or open a proposed one — ask if that citation is missing. Your contribution is still valuable to us, and we'd love to see you contribute again in the future! If you'd like feedback on how to improve this PR or want to discuss alternative approaches, please don't hesitate to reach out. Thank you for being part of the Omi community! |
Summary
Fixes #4375 — click-to-talk / chat-answer notifications showed only a tiny truncated body, and tapping them opened the app without navigating to the chat answer.
Root cause
notification+datapushes, so the shade only showed a short preview.navigate_topayload, and FCM open handlers needed to route/chat/{app}.Fix
send_client_displayed_notification(data on Android + APNS alert on iOS) withpush_type=chat_answer, fulltitle/body, andnavigate_to.ChatAnswerNotificationHandlercreates AwesomeNotifications withNotificationLayout.BigTextandnavigate_to/message_idpayload.notification_service_fcm.dart(includingonMessageOpenedApp/getInitialMessage→HomePageWrapper(navigateToRoute:)).fcm_background_handler.dartprovides a ChatAnswer-aware FCM entrypoint;notification_service_fcm.dartre-registers it aftermain.dart's sync handler so BigText still works when the app is not in the foreground.Files
app/lib/services/notifications/chat_answer_notification_handler.dart(new)app/lib/services/notifications/fcm_background_handler.dart(new)app/lib/services/notifications/notification_service_fcm.dartapp/test/unit/chat_answer_notification_handler_test.dart(new)backend/utils/chat_answer_notifications.py(new)backend/utils/chat.pybackend/tests/unit/test_chat_answer_client_displayed_notification.py(new)Notes
BasedHardware/omi:mainmay be needed before merge.send_app_notificationstill uses the legacysend_notificationpath on this fork snapshot; click-to-talk answers use the new client-displayed path viachat.py.Test plan
ChatAnswerNotificationHandler.isChatAnswerDatapush_type/chat/{app}with the answer