Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

android: post / dm handoff from push #4395

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

davidisaaclee
Copy link
Contributor

@davidisaaclee davidisaaclee commented Feb 6, 2025

extends #4280 to Android

This work was complicated by our custom Android notification handling code. Notably, we are intercepting notifications before expo-notifications has a chance to handle them, which I think means that we can't rely on expo-notifications' guarantees when receiving events or accessing notifications via JS (this is backed up by missing fields in the expo-notifications JS payloads, as if expo-notifications didn't have a chance to add the expected trigger data before JS could consume it).

more details about how we're breaking expo-notification's guarantees

I'm new to Android notifications, but here's my understanding:

Summary

  • Unroll async pyramid is simple code movement – excluding it from the diff might make review easier.
  • I could not figure out how to match the post/dmPost notification payloads that iOS adds (added inline comments about complications), so I put this data as JSON strings in the notification payload on Android
  • I found that the app was hitting hard errors when indexing into NotificationRequest.trigger on Android – expo-notifications' types says this trigger should never be null, but it is null on Android. This could either be an issue within expo-notifications (I didn't find anything in their issues in a quick search), or it could be due to how we manipulate notifications, which goes against expo-notifications' recommendation. To quickfix, I added cases to handle a null trigger to avoid crash.
  • Only clear channel notifications when have an active session – this felt like a necessary fix, as message notifications were being cleared before the user was able to read them in-app:
    1. User gets some notifications for new messages
    2. User opens app
    3. Code removes notifications for any channel which has an unread count of 0: if the channel with the new messages previously had no unreads, the code would immediately clear the notifications for the new messages before we had a chance to receive the message over sync
      To fix, hold off on clearing notifications unless we have an active session.

How did I test?

  1. Sent 2 messages to a user on an Android client while Android app was backgrounded
  2. Checked that Android client received pushes
  3. Opened Android client, navigated to channel with new messages
  4. Saw the 2 new messages load instantly, and the rest of the channel loaded subsequently
  5. Sent some more messages in this channel while app was open – new messages acted as expected

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant