Fix handling of duplicate and out-of-order notifications in WebUI (#19693)

* Fix handling of duplicate notifications from streaming server

* Fix handling of duplicate and out-of-order notifications when polling/expanding

Fixes #19615

app/javascript/mastodon/reducers/notifications.js

@@ -58,6 +58,11 @@ const notificationToMap = notification => ImmutableMap({
 const normalizeNotification = (state, notification, usePendingItems) => {
   const top = state.get('top');
 
+  // Under currently unknown conditions, the client may receive duplicates from the server
+  if (state.get('pendingItems').some((item) => item?.get('id') === notification.id) || state.get('items').some((item) => item?.get('id') === notification.id)) {
+    return state;
+  }
+
   if (usePendingItems || !state.get('pendingItems').isEmpty()) {
     return state.update('pendingItems', list => list.unshift(notificationToMap(notification))).update('unread', unread => unread + 1);
   }
@@ -77,28 +82,74 @@ const normalizeNotification = (state, notification, usePendingItems) => {
   });
 };
 
-const expandNormalizedNotifications = (state, notifications, next, isLoadingRecent, usePendingItems) => {
+const expandNormalizedNotifications = (state, notifications, next, isLoadingMore, isLoadingRecent, usePendingItems) => {
-  const lastReadId = state.get('lastReadId');
+  // This method is pretty tricky because:
-  let items = ImmutableList();
+  // - existing notifications might be out of order
+  // - the existing notifications may have gaps, most often explicitly noted with a `null` item
+  // - ideally, we don't want it to reorder existing items
+  // - `notifications` may include items that are already included
+  // - this function can be called either to fill in a gap, or load newer items
 
-  notifications.forEach((n, i) => {
+  const lastReadId = state.get('lastReadId');
-    items = items.set(i, notificationToMap(n));
+  const newItems = ImmutableList(notifications.map(notificationToMap));
-  });
 
   return state.withMutations(mutable => {
-    if (!items.isEmpty()) {
+    if (!newItems.isEmpty()) {
       usePendingItems = isLoadingRecent && (usePendingItems || !mutable.get('pendingItems').isEmpty());
 
-      mutable.update(usePendingItems ? 'pendingItems' : 'items', list => {
+      mutable.update(usePendingItems ? 'pendingItems' : 'items', oldItems => {
-        const lastIndex = 1 + list.findLastIndex(
+        // If called to poll *new* notifications, we just need to add them on top without duplicates
-          item => item !== null && (compareId(item.get('id'), items.last().get('id')) > 0 || item.get('id') === items.last().get('id')),
+        if (isLoadingRecent) {
-        );
+          const idsToCheck = oldItems.map(item => item?.get('id')).toSet();
+          const insertedItems = newItems.filterNot(item => idsToCheck.includes(item.get('id')));
+          return insertedItems.concat(oldItems);
+        }
 
-        const firstIndex = 1 + list.take(lastIndex).findLastIndex(
+        // If called to expand more (presumably older than any known to the WebUI), we just have to
-          item => item !== null && compareId(item.get('id'), items.first().get('id')) > 0,
+        // add them to the bottom without duplicates
-        );
+        if (isLoadingMore) {
+          const idsToCheck = oldItems.map(item => item?.get('id')).toSet();
+          const insertedItems = newItems.filterNot(item => idsToCheck.includes(item.get('id')));
+          return oldItems.concat(insertedItems);
+        }
 
-        return list.take(firstIndex).concat(items, list.skip(lastIndex));
+        // Now this gets tricky, as we don't necessarily know for sure where the gap to fill is,
+        // and some items in the timeline may not be properly ordered.
+
+        // However, we know that `newItems.last()` is the oldest item that was requested and that
+        // there is no “hole” between `newItems.last()` and `newItems.first()`.
+
+        // First, find the furthest (if properly sorted, oldest) item in the notifications that is
+        // newer than the oldest fetched one, as it's most likely that it delimits the gap.
+        // Start the gap *after* that item.
+        const lastIndex = oldItems.findLastIndex(item => item !== null && compareId(item.get('id'), newItems.last().get('id')) >= 0) + 1;
+
+        // Then, try to find the furthest (if properly sorted, oldest) item in the notifications that
+        // is newer than the most recent fetched one, as it delimits a section comprised of only
+        // items older or within `newItems` (or that were deleted from the server, so should be removed
+        // anyway).
+        // Stop the gap *after* that item.
+        const firstIndex = oldItems.take(lastIndex).findLastIndex(item => item !== null && compareId(item.get('id'), newItems.first().get('id')) > 0) + 1;
+
+        // At this point:
+        // - no `oldItems` after `firstIndex` is newer than any of the `newItems`
+        // - all `oldItems` after `lastIndex` are older than every of the `newItems`
+        // - it is possible for items in the replaced slice to be older than every `newItems`
+        // - it is possible for items before `firstIndex` to be in the `newItems` range
+        // Therefore:
+        // - to avoid losing items, items from the replaced slice that are older than `newItems`
+        //   should be added in the back.
+        // - to avoid duplicates, `newItems` should be checked the first `firstIndex` items of
+        //   `oldItems`
+        const idsToCheck = oldItems.take(firstIndex).map(item => item?.get('id')).toSet();
+        const insertedItems = newItems.filterNot(item => idsToCheck.includes(item.get('id')));
+        const olderItems = oldItems.slice(firstIndex, lastIndex).filter(item => item !== null && compareId(item.get('id'), newItems.last().get('id')) < 0);
+
+        return oldItems.take(firstIndex).concat(
+          insertedItems,
+          olderItems,
+          oldItems.skip(lastIndex),
+        );
       });
     }
 
@@ -109,7 +160,7 @@ const expandNormalizedNotifications = (state, notifications, next, isLoadingRece
     if (shouldCountUnreadNotifications(state)) {
       mutable.set('unread', mutable.get('pendingItems').count(item => item !== null) + mutable.get('items').count(item => item && compareId(item.get('id'), lastReadId) > 0));
     } else {
-      const mostRecent = items.find(item => item !== null);
+      const mostRecent = newItems.find(item => item !== null);
       if (mostRecent && compareId(lastReadId, mostRecent.get('id')) < 0) {
         mutable.set('lastReadId', mostRecent.get('id'));
       }
@@ -224,7 +275,7 @@ export default function notifications(state = initialState, action) {
   case NOTIFICATIONS_UPDATE:
     return normalizeNotification(state, action.notification, action.usePendingItems);
   case NOTIFICATIONS_EXPAND_SUCCESS:
-    return expandNormalizedNotifications(state, action.notifications, action.next, action.isLoadingRecent, action.usePendingItems);
+    return expandNormalizedNotifications(state, action.notifications, action.next, action.isLoadingMore, action.isLoadingRecent, action.usePendingItems);
   case ACCOUNT_BLOCK_SUCCESS:
     return filterNotifications(state, [action.relationship.id]);
   case ACCOUNT_MUTE_SUCCESS: