From 7c8e2b9859f2e206110accb74d19ec4591d79780 Mon Sep 17 00:00:00 2001 From: Claire Date: Fri, 4 Nov 2022 00:14:39 +0100 Subject: [PATCH] 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 --- .../mastodon/reducers/notifications.js | 85 +++++++++++++++---- 1 file changed, 68 insertions(+), 17 deletions(-) diff --git a/app/javascript/mastodon/reducers/notifications.js b/app/javascript/mastodon/reducers/notifications.js index eb34edb633..eb5368198c 100644 --- a/app/javascript/mastodon/reducers/notifications.js +++ b/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 lastReadId = state.get('lastReadId'); - let items = ImmutableList(); +const expandNormalizedNotifications = (state, notifications, next, isLoadingMore, isLoadingRecent, usePendingItems) => { + // This method is pretty tricky because: + // - 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) => { - items = items.set(i, notificationToMap(n)); - }); + const lastReadId = state.get('lastReadId'); + 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 => { - const lastIndex = 1 + list.findLastIndex( - item => item !== null && (compareId(item.get('id'), items.last().get('id')) > 0 || item.get('id') === items.last().get('id')), - ); + mutable.update(usePendingItems ? 'pendingItems' : 'items', oldItems => { + // If called to poll *new* notifications, we just need to add them on top without duplicates + 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( - item => item !== null && compareId(item.get('id'), items.first().get('id')) > 0, - ); + // If called to expand more (presumably older than any known to the WebUI), we just have to + // 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: