From 43d099836ba7b44c93a190a8b9773b75e6271217 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Fri, 1 Mar 2019 15:48:10 -0700 Subject: [PATCH 1/2] Always insert rooms into lists when they get lost Room upgrades, direct chats, etc all end up being lost in these scenarios. Instead of losing them to the list, try and put them into a relevant spot of the list. Fixes https://github.com/vector-im/riot-web/issues/9020 --- src/stores/RoomListStore.js | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/src/stores/RoomListStore.js b/src/stores/RoomListStore.js index e9ac33b506..fc140f31e6 100644 --- a/src/stores/RoomListStore.js +++ b/src/stores/RoomListStore.js @@ -477,18 +477,12 @@ class RoomListStore extends Store { room, category, this._state.lists[key], listsClone[key], lastTimestamp); if (!pushedEntry) { - // Special case invites: they don't really have timelines and can easily get lost when - // the user has multiple pending invites. Pushing them is the least worst option. - if (listsClone[key].length === 0 || key === "im.vector.fake.invite") { - listsClone[key].push({room, category}); - insertedIntoTags.push(key); - } else { - // In theory, this should never happen - console.warn(`!! Room ${room.roomId} lost: No position available`); - } - } else { - insertedIntoTags.push(key); + // There's some circumstances where the room doesn't fit anywhere, so just + // push the room in. We push it in at the start of the list because the room + // is probably important. + listsClone[key].splice(0, 0, {room, category}); } + insertedIntoTags.push(key); } } From 49f506cef49e14e6c7b2adec71f0076388e25c56 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Fri, 1 Mar 2019 17:18:16 -0700 Subject: [PATCH 2/2] More clearly fix issues with room insertion to lists Instead of having a catch-all insert, try and fix the common cases with a bit more care. --- src/stores/RoomListStore.js | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/src/stores/RoomListStore.js b/src/stores/RoomListStore.js index fc140f31e6..78e0659e36 100644 --- a/src/stores/RoomListStore.js +++ b/src/stores/RoomListStore.js @@ -429,6 +429,13 @@ class RoomListStore extends Store { newList.push(entry); } + if (!pushedEntry && desiredCategoryBoundaryIndex >= 0) { + console.warn(`!! Room ${room.roomId} nearly lost: Ran off the end of the list`); + console.warn(`!! Inserting at position ${desiredCategoryBoundaryIndex} with category ${category}`); + newList.splice(desiredCategoryBoundaryIndex, 0, {room, category}); + pushedEntry = true; + } + return pushedEntry; } @@ -477,16 +484,27 @@ class RoomListStore extends Store { room, category, this._state.lists[key], listsClone[key], lastTimestamp); if (!pushedEntry) { - // There's some circumstances where the room doesn't fit anywhere, so just - // push the room in. We push it in at the start of the list because the room - // is probably important. + // This should rarely happen: _slotRoomIntoList has several checks which attempt + // to make sure that a room is not lost in the list. If we do lose the room though, + // we shouldn't throw it on the floor and forget about it. Instead, we should insert + // it somewhere. We'll insert it at the top for a couple reasons: 1) it is probably + // an important room for the user and 2) if this does happen, we'd want a bug report. + console.warn(`!! Room ${room.roomId} nearly lost: Failed to find a position`); + console.warn(`!! Inserting at position 0 in the list and flagging as inserted`); + console.warn("!! Additional info: ", { + category, + key, + upToIndex: listsClone[key].length, + expectedCount: this._state.lists[key].length, + }); listsClone[key].splice(0, 0, {room, category}); } insertedIntoTags.push(key); } } - // Double check that we inserted the room in the right places + // Double check that we inserted the room in the right places. + // There should never be a discrepancy. for (const targetTag of targetTags) { let count = 0; for (const insertedTag of insertedIntoTags) {