Fix missing/swapped/duplicated messages due to wrong TimelineChunk modifications or insertions #5528
Conversation
I was observing cases where builtEvents[modificationIndex] was not having the same eventId as the udpatedEntity in handleDatabaseChangeSet. In particular, I observed both cases that - there was no item in the list yet with the same eventId as the updated one - there was an item with the same eventId already in the list, but at a different position. Whenever this happened, the timeline would render missing, duplicated, or swapped messages in the timeline. Instead of relying on the modificationIndex to be the same for both the change set and builtEvents, look up the proper index by eventId.
|
Example of what this is trying to fix: see missing message 97 with
|
|
I think I just observed a case which fits this prediction of mine:
I.e., I just had a case where the chunk had all events until displayIndex 11 loaded, and then |
ok, found a way to reproduce: just reduce initial loaded chunk size and pagination size, artificially delay chunk loading from code, and open the timeline somewhere in the past to get some slow forwards loading going on. Now if new messages arrive, these are fast-forwarded before the rest of the chunk is loaded, resulting in skipped messages (and preventing the chunk to load the missed messages). Pushing a fix as another commit. Note I was only able to test the case where insertions where at the beginning so far, not sure how test the other case. |
SpiritCroc
changed the title
|
Thanks for the PR. I defer to @ganfra to review it. |
| // Check consistency to item before insertions | ||
| if (range.startIndex > 0) { | ||
| val firstInsertion = results[range.startIndex]!! | ||
| val lastBeforeInsertion = builtEvents[range.startIndex - 1] |
There was a problem hiding this comment.
If there is no built event yet, this could crash
There was a problem hiding this comment.
There is builtEvents.isNotEmpty() in line 435
There was a problem hiding this comment.
Oh yeah; I'm still wandering if we should try to fix the underneath issue here instead of doing some weird checks :/
|
@ganfra can we merge this PR? |
bmarty
requested review from
a team and
ariskotsomitopoulos
and removed request for
a team
| if (builtEvents.isNotEmpty()) { | ||
| // Check consistency to item before insertions | ||
| if (range.startIndex > 0) { | ||
| val firstInsertion = results[range.startIndex]!! |
There was a problem hiding this comment.
I would avoid !! even if we are sure it cannot be null
I was observing cases where
builtEvents[modificationIndex]was nothaving the same
eventIdas theudpatedEntityinhandleDatabaseChangeSet().In particular, I observed both cases that
eventIdas the updatedone
eventIdalready in the list, but at adifferent position.
Whenever this happened, the timeline would render missing, duplicated,
or swapped messages in the timeline.
Instead of relying on
modificationIndexto be the same for both thechange set and
builtEvents, look up the proper index byeventId.Note: I'm not entirely sure if this is the correct way to fix this.Is the previously used assumption that
displayIndex == modificationIndexsomething that should hold?If yes, this PR is likely not the correct fix.
If no, could the insertions in
handleDatabaseChangeSet()also be broken, since I'm only addressing modification's indices here?For insertions, inconsistencies could also occur, if the chunk was not fully loaded yet, so check this as well.
Signed-off-by: Tobias Büttner [email protected]
Type of change
Content
Look up indices for event modifications, instead of using the ones from the change set.
Motivation and context
Messages were sometimes showing swapped, duplicated, or missing.
Screenshots / GIFs
Tests
Observe if messages are rendered correctly. I also used following commits to further debug this:
Tested devices
Checklist