Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Always insert rooms into lists when they get lost #2736

Merged
merged 2 commits into from
Mar 2, 2019
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 24 additions & 12 deletions src/stores/RoomListStore.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down Expand Up @@ -477,22 +484,27 @@ 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`);
Copy link
Collaborator

@jryans jryans Mar 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth keeping some kind of logging that it would have been lost without this new fallback? (Do we hope to do something better in the future?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely - have added a bunch of logging

}
} else {
insertedIntoTags.push(key);
// 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) {
Expand Down