More notifications improvements (#2198)

* On mobile, never replace the notifs under the user due to focus events

* Use the server's seenAt response to calculate isRead state locally

package.json

@@ -35,7 +35,7 @@
     "intl:compile": "lingui compile"
   },
   "dependencies": {
-    "@atproto/api": "^0.7.2",
+    "@atproto/api": "^0.7.3",
     "@bam.tech/react-native-image-resizer": "^3.0.4",
     "@braintree/sanitize-url": "^6.0.2",
     "@emoji-mart/react": "^1.1.1",

src/state/queries/notifications/feed.ts

@@ -16,7 +16,7 @@
  * 3. Don't call this query's `refetch()` if you're trying to sync latest; call `checkUnread()` instead.
  */
 
-import {useEffect, useRef} from 'react'
+import {useEffect} from 'react'
 import {AppBskyFeedDefs} from '@atproto/api'
 import {
   useInfiniteQuery,
@@ -49,8 +49,6 @@ export function useNotificationFeedQuery(opts?: {enabled?: boolean}) {
   const threadMutes = useMutedThreads()
   const unreads = useUnreadNotificationsApi()
   const enabled = opts?.enabled !== false
-  // state tracked across page fetches
-  const pageState = useRef({pageNum: 0, hasMarkedRead: false})
 
   const query = useInfiniteQuery<
     FeedPage,
@@ -66,8 +64,6 @@ export function useNotificationFeedQuery(opts?: {enabled?: boolean}) {
       if (!pageParam) {
         // for the first page, we check the cached page held by the unread-checker first
         page = unreads.getCachedUnreadPage()
-        // reset the page state
-        pageState.current = {pageNum: 0, hasMarkedRead: false}
       }
       if (!page) {
         page = await fetchPage({
@@ -80,27 +76,9 @@ export function useNotificationFeedQuery(opts?: {enabled?: boolean}) {
         })
       }
 
-      // NOTE
+      // if the first page has an unread, mark all read
-      // this section checks to see if we need to mark notifs read
+      if (!pageParam && page.items[0] && !page.items[0].notification.isRead) {
-      // we want to wait until we've seen a read notification because
-      // of a timing challenge; marking read on the first page would
-      // cause subsequent pages of unread notifs to incorrectly come
-      // back as "read". we use page 6 as an abort condition, which means
-      // after ~180 notifs we give up on tracking unread state correctly
-      // -prf
-      if (!pageState.current.hasMarkedRead) {
-        let hasMarkedRead = false
-        if (
-          pageState.current.pageNum > 5 ||
-          page.items.some(item => item.notification.isRead)
-        ) {
         unreads.markAllRead()
-          hasMarkedRead = true
-        }
-        pageState.current = {
-          pageNum: pageState.current.pageNum + 1,
-          hasMarkedRead,
-        }
       }
 
       return page
@@ -108,6 +86,20 @@ export function useNotificationFeedQuery(opts?: {enabled?: boolean}) {
     initialPageParam: undefined,
     getNextPageParam: lastPage => lastPage.cursor,
     enabled,
+    select(data: InfiniteData<FeedPage>) {
+      // override 'isRead' using the first page's returned seenAt
+      // we do this because the `markAllRead()` call above will
+      // mark subsequent pages as read prematurely
+      const seenAt = data.pages[0]?.seenAt || new Date()
+      for (const page of data.pages) {
+        for (const item of page.items) {
+          item.notification.isRead =
+            seenAt > new Date(item.notification.indexedAt)
+        }
+      }
+
+      return data
+    },
   })
 
   useEffect(() => {

src/state/queries/notifications/types.ts

@@ -24,6 +24,7 @@ export interface FeedNotification {
 
 export interface FeedPage {
   cursor: string | undefined
+  seenAt: Date
   items: FeedNotification[]
 }
 

src/state/queries/notifications/util.ts

@@ -68,8 +68,14 @@ export async function fetchPage({
     notif => !isThreadMuted(notif, threadMutes),
   )
 
+  let seenAt = res.data.seenAt ? new Date(res.data.seenAt) : new Date()
+  if (Number.isNaN(seenAt.getTime())) {
+    seenAt = new Date()
+  }
+
   return {
     cursor: res.data.cursor,
+    seenAt,
     items: notifsGrouped,
   }
 }

src/view/screens/Notifications.tsx

@@ -67,8 +67,8 @@ export function NotificationsScreen({}: Props) {
   const onFocusCheckLatest = React.useCallback(() => {
     // on focus, check for latest, but only invalidate if the user
     // isnt scrolled down to avoid moving content underneath them
-    unreadApi.checkUnread({invalidate: !isScrolledDown})
+    unreadApi.checkUnread({invalidate: !isScrolledDown && isDesktop})
-  }, [unreadApi, isScrolledDown])
+  }, [unreadApi, isScrolledDown, isDesktop])
   checkLatestRef.current = onFocusCheckLatest
 
   // on-visible setup

yarn.lock

@@ -48,6 +48,20 @@
     typed-emitter "^2.1.0"
     zod "^3.21.4"
 
+"@atproto/api@^0.7.3":
+  version "0.7.3"
+  resolved "https://registry.yarnpkg.com/@atproto/api/-/api-0.7.3.tgz#3224000353619970d5e397a157c6e189e195ef47"
+  integrity sha512-fKU+W+S4kKxClE6IcPBHPZAjcyBYxG28S0FW/bv3T/ZYDkNxGzDV4xuoHOyEDGtB30slltl5U83njuuRZs5xtw==
+  dependencies:
+    "@atproto/common-web" "^0.2.3"
+    "@atproto/lexicon" "^0.3.1"
+    "@atproto/syntax" "^0.1.5"
+    "@atproto/xrpc" "^0.4.1"
+    multiformats "^9.9.0"
+    tlds "^1.234.0"
+    typed-emitter "^2.1.0"
+    zod "^3.21.4"
+
 "@atproto/aws@^0.1.6":
   version "0.1.6"
   resolved "https://registry.yarnpkg.com/@atproto/aws/-/aws-0.1.6.tgz#c6ecbfd92b325f3c5433688534d47f43358b415b"