Handle push notifications for DMs #3895

haileyok · 2024-05-07T16:45:56Z

Why

Edit

For now, let's merge this in with only the DM notification logic, since that is the primary thing we need right now. We should investigate other platforms and determine how we want to handle each individual case here. I.e. where do we want to push for a certain type of notification.

This needed a good bit of thinking to get right, but I think it is mostly there. We're tackling a few birds with one stone here!

Pressing a mention or reply notification takes you to that post rather than the notifications tab
Pressing a follow notification takes you to that profile
DM notifications will (currently) alway show up in the app. Unless...
Notifications will not be displayed for the current conversation you are viewing
When the app is foregrounded, you will not see notifications for messages on accounts other than the one you're currently actively signed in to.
When in the background, you will see notifications for all accounts. If you tap on a notification for an account that is not your currently signed-in account, you'll be automatically switched to the other account and then pushed to the conversation.

Note about points 3 and 4

We need to consider one thing when it comes to points one and two here. When you open the app through one of these notifications, we won't actually "remove" these items from your "unread" notifications. This is a tricky thing to do, because we are currently making notifications as read with a "last seen" time. If we updated that time when you pressed a notification for a mention, we'd be wiping all your unreads up to that point - not just the one you're tapping.

With that said, we can comment out those handlers for now. It sort of comes down to "which is the better experience" I think right now. We'll talk about how to improve this as well, but it was a super easy thing to add in here where I needed to rework the handler anyway.

Test Plan

Testing this is not fun. There isn't any way to receive notifications on the simulator, and even if you build for a device, you still cannot receive the notifications as the push notification token is a development token rather than a production one. With that said, you can send "fake" notifications.

Figure out the device name - xcrun simctl list devices | grep Booted
Save a payload to testnotif.json. An example payload would be

{
    "aps": {
        "alert": {
            "title": "Mention Notification",
            "body": "A test Mention notification"
        }
    },
    "reason": "mention",
    "uri": "at://did:plc:2p25gewthftad2wefe7yyzhr/app.bsky.feed.post/3krxmsj45iw2x",
    "subject": ""
}

Send the notification - xcrun simctl push "iPhone 15 Pro" xyz.blueskyweb.app testMention.json

Pressing a mention or reply notification takes you to the post

Those types of notifications still do not show up when the app is foregrounded

Screen.Recording.2024-05-08.at.12.20.22.PM.mov

Pressing a follow notification takes you to the profile

Pressing a message notification takes you to the convo

Pressing a notification for an account that isn't the current one switches your account

render · 2024-05-07T16:46:00Z

Your Render PR Server URL is https://social-app-pr-3895.onrender.com.

Follow its progress at https://dashboard.render.com/web/srv-cot5lhg21fec739ehsc0.

github-actions · 2024-05-07T16:50:59Z

Old size	New size	Diff
6.89 MB	6.89 MB	196 B (0.00%)

prep merge move `useNotificationsListener` into shell progress better structure only show messages notifications while using app if it is the current account progress only emit on native current chat emitter only show alerts for the current chat type add logs setup handlers

haileyok · 2024-05-08T23:04:42Z

src/lib/hooks/useNotificationHandler.ts

+          setTimeout(() => {
+            // @ts-expect-error types are weird here
+            navigation.navigate('MessagesTab', {
+              screen: 'MessagesConversation',
+              params: {
+                conversation: payload.convoId,
+              },
+            })
+          }, 500)


The setTimeouts for .navigate in this code are not "necessary", but they have a better feel then the very immediate navigation.

Another thing that I'd like to look at here is handling adding the Messages screen onto this stack if it doesn't exist already. The react nav documentation is a bit unclear on how to do this, so maybe I can find an example elsewhere in the codebase.

Yeah having Messages as the immediate screen when pressing "back" would be awesome

haileyok · 2024-05-08T23:05:37Z

src/lib/hooks/useNotificationHandler.ts

+    Notifications.setNotificationHandler({
+      handleNotification: async e => {
+        if (e.request.trigger.type !== 'push') return DEFAULT_HANDLER_OPTIONS
+
+        logger.debug(
+          'Notifications: received',
+          {e},
+          logger.DebugContext.notifications,
+        )
+
+        const payload = e.request.trigger.payload as NotificationRecord
+        if (
+          payload.reason === 'chat-message' &&
+          payload.recipientDid === currentAccount?.did &&
+          currentConvoId !== payload.convoId
+        ) {
+          return {
+            shouldShowAlert: true,
+            shouldPlaySound: false,
+            shouldSetBadge: false,
+          }
+        }
+
+        // Any notification other than a chat message should invalidate the unread page
+        invalidateCachedUnreadPage()
+        return DEFAULT_HANDLER_OPTIONS
+      },
+    })


This replaces the old addNotificationReceivedListener, because it lets us also control visibility of the push notification.

haileyok · 2024-05-08T23:08:27Z

src/lib/hooks/useNotificationHandler.ts

+    if (
+      storedPayload?.reason === 'chat-message' &&
+      currentAccount?.did === storedPayload.recipientDid
+    ) {
+      handleNotification(storedPayload)
+      storedPayload = undefined
+    }


When we encounter a case where you open a notification that doesn't belong to the currently signed in account, we switch the account above and store the payload. Once currentAccount changes, we will re-handle the notification.

This only applies to chat-message right now because we don't have a recipientDid yet on other notification payloads.

haileyok · 2024-05-08T23:10:37Z

src/screens/Messages/Conversation/index.tsx

+      navigation.reset({
+        index: 0,
+        routes: [{name: 'Messages'}],
+      })


For now, in cases where we got to this screen from a notification, adding the Messages route to the start of this creates the "pop" animation that we are looking for.

estrattonbailey

Code LGTM, pending discussion of behaviors

estrattonbailey · 2024-05-09T01:46:50Z

src/lib/hooks/useNotificationHandler.ts

+type NotificationReason =
+  | 'like'
+  | 'repost'
+  | 'follow'
+  | 'mention'
+  | 'reply'
+  | 'quote'
+  | 'chat-message'
+
+type NotificationRecord =
+  | {
+      reason: Exclude<NotificationReason, 'chat-message'>
+      uri: string
+      subject: string
+    }
+  | {
+      reason: 'chat-message'
+      convoId: string
+      messageId: string
+      recipientDid: string
+    }


sickos-yes.png

estrattonbailey · 2024-05-09T01:51:45Z

src/lib/hooks/useNotificationHandler.ts

+          setTimeout(() => {
+            // @ts-expect-error types are weird here
+            navigation.navigate('MessagesTab', {
+              screen: 'MessagesConversation',
+              params: {
+                conversation: payload.convoId,
+              },
+            })
+          }, 500)


Yeah having Messages as the immediate screen when pressing "back" would be awesome

estrattonbailey · 2024-05-09T01:57:33Z

src/lib/hooks/useNotificationHandler.ts

+    if (
+      storedPayload?.reason === 'chat-message' &&
+      currentAccount?.did === storedPayload.recipientDid
+    ) {
+      handleNotification(storedPayload)
+      storedPayload = undefined
+    }


* origin/main: Handle push notifications for DMs (#3895) Add gate, a:a swap onboarding state (#3930) [🐴] Add hover context menu for convo list on web (#3923)

haileyok force-pushed the hailey/dm-push-notifs branch from ead83b0 to 417de2b Compare May 8, 2024 05:31

haileyok added 7 commits May 7, 2024 22:37

merge

e38e8c8

remove event emitter

4fe7808

just needs cleanup

92f710c

oops

cfe8e64

remove unnecessary queryClient param

e7f1896

few fixes

3b30791

cleanup

2b18bbd

haileyok marked this pull request as ready for review May 8, 2024 17:58

haileyok added 5 commits May 8, 2024 10:59

nit

d632670

remove folds

f9b1f86

remove comment

1b95956

simplify if

bd024b2

add back invalidate

fabaad4

haileyok commented May 8, 2024

View reviewed changes

estrattonbailey reviewed May 9, 2024

View reviewed changes

haileyok added 8 commits May 8, 2024 20:12

Merge branch 'main' into hailey/dm-push-notifs

e968456

comment out other navigations for now

cf13ec9

rename type

1ed0ed5

handle various navigation cases

d4d252f

push to conversation from notification

6c93b5b

update badge in all cases except chat-message

6b4eccf

ensure no duplicate notifications

27810d2

rm unused animationOnReplace

70d1056

haileyok requested a review from estrattonbailey May 9, 2024 16:20

estrattonbailey approved these changes May 9, 2024

View reviewed changes

revert to using goBack in the conversation header

9065b13

haileyok changed the title ~~Better push notification handling~~ Handle push notifications for DMs May 9, 2024

add todo comment

f31ba3c

haileyok merged commit 17e3b94 into main May 9, 2024
6 checks passed

haileyok deleted the hailey/dm-push-notifs branch May 9, 2024 17:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle push notifications for DMs #3895

Handle push notifications for DMs #3895

haileyok commented May 7, 2024 •

edited

Loading

render bot commented May 7, 2024

github-actions bot commented May 7, 2024 •

edited

Loading

haileyok May 8, 2024

haileyok May 8, 2024

estrattonbailey May 9, 2024

haileyok May 8, 2024

haileyok May 8, 2024

estrattonbailey May 9, 2024

haileyok May 8, 2024

estrattonbailey left a comment

estrattonbailey May 9, 2024

estrattonbailey May 9, 2024

estrattonbailey May 9, 2024

Handle push notifications for DMs #3895

Handle push notifications for DMs #3895

Conversation

haileyok commented May 7, 2024 • edited Loading

Why

Edit

Note about points 3 and 4

Test Plan

Pressing a mention or reply notification takes you to the post

Those types of notifications still do not show up when the app is foregrounded

Pressing a follow notification takes you to the profile

Pressing a message notification takes you to the convo

Pressing a notification for an account that isn't the current one switches your account

render bot commented May 7, 2024

github-actions bot commented May 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estrattonbailey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haileyok commented May 7, 2024 •

edited

Loading

github-actions bot commented May 7, 2024 •

edited

Loading