From dd4c156f33a24b8bb89b45b2697aa4036c3ae5be Mon Sep 17 00:00:00 2001 From: Claire Date: Wed, 6 Apr 2022 21:01:41 +0200 Subject: [PATCH] Fix possible duplicate statuses in timelines in some edge cases (#17971) In some rare cases, when receiving statuses out of order from the streaming API then polling from the REST API, it was possible for the `expandNormalizedTimeline` function to insert duplicates in the timeline, which would then result in several bugs. This commits ensures that there are no duplicates inserted in the timeline. --- app/javascript/mastodon/reducers/timelines.js | 39 +++++++++++++++---- 1 file changed, 32 insertions(+), 7 deletions(-) diff --git a/app/javascript/mastodon/reducers/timelines.js b/app/javascript/mastodon/reducers/timelines.js index b66c19fd5..301997567 100644 --- a/app/javascript/mastodon/reducers/timelines.js +++ b/app/javascript/mastodon/reducers/timelines.js @@ -16,7 +16,7 @@ import { ACCOUNT_MUTE_SUCCESS, ACCOUNT_UNFOLLOW_SUCCESS, } from '../actions/accounts'; -import { Map as ImmutableMap, List as ImmutableList, fromJS } from 'immutable'; +import { Map as ImmutableMap, List as ImmutableList, OrderedSet as ImmutableOrderedSet, fromJS } from 'immutable'; import compareId from '../compare_id'; const initialState = ImmutableMap(); @@ -32,6 +32,13 @@ const initialTimeline = ImmutableMap({ }); const expandNormalizedTimeline = (state, timeline, statuses, next, isPartial, isLoadingRecent, usePendingItems) => { + // This method is pretty tricky because: + // - existing items in the timeline might be out of order + // - the existing timeline may have gaps, most often explicitly noted with a `null` item + // - ideally, we don't want it to reorder existing items of the timeline + // - `statuses` may include items that are already included in the timeline + // - this function can be called either to fill in a gap, or load newer items + return state.update(timeline, initialTimeline, map => map.withMutations(mMap => { mMap.set('isLoading', false); mMap.set('isPartial', isPartial); @@ -46,15 +53,33 @@ const expandNormalizedTimeline = (state, timeline, statuses, next, isPartial, is mMap.update(usePendingItems ? 'pendingItems' : 'items', ImmutableList(), oldIds => { const newIds = statuses.map(status => status.get('id')); - const lastIndex = oldIds.findLastIndex(id => id !== null && compareId(id, newIds.last()) >= 0) + 1; - const firstIndex = oldIds.take(lastIndex).findLastIndex(id => id !== null && compareId(id, newIds.first()) > 0); + // 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. - if (firstIndex < 0) { - return (isPartial ? newIds.unshift(null) : newIds).concat(oldIds.skip(lastIndex)); + // However, we know that `newIds.last()` is the oldest item that was requested and that + // there is no “hole” between `newIds.last()` and `newIds.first()`. + + // First, find the furthest (if properly sorted, oldest) item in the timeline 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 = oldIds.findLastIndex(id => id !== null && compareId(id, newIds.last()) >= 0) + 1; + + // Then, try to find the furthest (if properly sorted, oldest) item in the timeline that + // is newer than the most recent fetched one, as it delimits a section comprised of only + // items present in `newIds` (or that were deleted from the server, so should be removed + // anyway). + // Stop the gap *after* that item. + const firstIndex = oldIds.take(lastIndex).findLastIndex(id => id !== null && compareId(id, newIds.first()) > 0) + 1; + + // Make sure we aren't inserting duplicates + let insertedIds = ImmutableOrderedSet(newIds).subtract(oldIds.take(firstIndex), oldIds.skip(lastIndex)).toList(); + // Finally, insert a gap marker if the data is marked as partial by the server + if (isPartial && (firstIndex === 0 || oldIds.get(firstIndex - 1) !== null)) { + insertedIds = insertedIds.unshift(null); } - return oldIds.take(firstIndex + 1).concat( - isPartial && oldIds.get(firstIndex) !== null ? newIds.unshift(null) : newIds, + return oldIds.take(firstIndex).concat( + insertedIds, oldIds.skip(lastIndex), ); });