Skip to content

fix(app,backend): BigText chat-answer notifications + chat deep-link - #13065

Closed
apoorvdarshan wants to merge 12 commits into
BasedHardware:mainfrom
apoorvdarshan:fix/4375-click-to-talk-notifications
Closed

fix(app,backend): BigText chat-answer notifications + chat deep-link#13065
apoorvdarshan wants to merge 12 commits into
BasedHardware:mainfrom
apoorvdarshan:fix/4375-click-to-talk-notifications

Conversation

@apoorvdarshan

@apoorvdarshan apoorvdarshan commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

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

  1. Truncation: Android system FCM trays use the default collapsed style (no BigText). Chat answers were sent as standard notification+data pushes, so the shade only showed a short preview.
  2. Weak deep-link on tap: Local AwesomeNotifications for plugin/chat answers did not consistently use BigText + a navigate_to payload, and FCM open handlers needed to route /chat/{app}.

Fix

  • Backend: Chat answer pushes go through send_client_displayed_notification (data on Android + APNS alert on iOS) with push_type=chat_answer, full title/body, and navigate_to.
  • Flutter: ChatAnswerNotificationHandler creates AwesomeNotifications with NotificationLayout.BigText and navigate_to / message_id payload.
  • Foreground: Wired in notification_service_fcm.dart (including onMessageOpenedApp / getInitialMessageHomePageWrapper(navigateToRoute:)).
  • Background/terminated: fcm_background_handler.dart provides a ChatAnswer-aware FCM entrypoint; notification_service_fcm.dart re-registers it after main.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.dart
  • app/test/unit/chat_answer_notification_handler_test.dart (new)
  • backend/utils/chat_answer_notifications.py (new)
  • backend/utils/chat.py
  • backend/tests/unit/test_chat_answer_client_displayed_notification.py (new)

Notes

  • Branch was cut from the fork; a rebase onto latest BasedHardware/omi:main may be needed before merge.
  • Plugin send_app_notification still uses the legacy send_notification path on this fork snapshot; click-to-talk answers use the new client-displayed path via chat.py.

Test plan

  • Unit: ChatAnswerNotificationHandler.isChatAnswerData
  • Unit: client-displayed FCM builder omits top-level notification / sets push_type
  • Device: click-to-talk → notification shows expanded answer text (BigText)
  • Device: tap notification → opens /chat/{app} with the answer
  • Device: same while app is backgrounded / terminated

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, causing navigate_to to 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 because testing.import_isolation is 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';

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Git-on-my-level left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. 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.
  2. 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.
  3. 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.
  4. 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.

@apoorvdarshan

Copy link
Copy Markdown
Contributor Author

Superseded by #13173 — clean reimplementation on current main addressing the CHANGES_REQUESTED review (async client-displayed path, NotificationUtil cold-start navigation, single main.dart background handler, extend utils/notifications.py instead of a parallel module, Cubic background/getInitialMessage/test import fixes).

Closing this PR as superseded; please review #13173 instead.

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

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:

  • Project standards — Ensuring consistency across the codebase
  • User needs — Making sure changes align with what our users need
  • Code best practices — Maintaining code quality and maintainability
  • Project direction — Keeping aligned with our product principles and locked invariants

Before your next PR, please skim:

  • PRODUCT.md — product north star
  • Product invariants — locked rules (shared chat, memory tiers, agent control plane, integrations, brand)

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

flutter flutter work needs-maintainer-review Needs a human maintainer to sign off before merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Click to talk notifications don't show full text and won't take me to the chat with that answer

2 participants