search

matrix-org / matrix-rust-sdk

Matrix Client-Server SDK for Rust
Apache License 2.0
1.26k stars 246 forks source link

paginating edited events in the timeline, shows the original content #3492

Closed Velin92 closed 3 weeks ago

Velin92 commented 5 months ago

Steps to reproduce

  1. Edit any old message
  2. Close and reopen the app (sometimes just leaving the room is enough)
  3. Scroll back to the old message (triggering pagination)

Outcome

What did you expect?

I read the new content of the edited message

What happened instead?

I read the old content of the edited message even if it appears as edited.

https://github.com/element-hq/element-x-ios/assets/34335419/869bf261-88ff-449b-aab1-ab1dd7a546c5

Related to the following issue: https://github.com/element-hq/element-x-ios/issues/2884

zecakeh commented 2 months ago

Does this happen only in encrypted rooms? If yes this could be the same as #2848, and should be mostly fixed now.

bnjbvr commented 2 months ago

I don't think it's restrictred to encrypted rooms. If the timeline sees an edit event before the event that's edited, it will behave like that; that happens when back-paginating, where events are added in the order they're received from the server, i.e. from the most recent to the least recent. An integration test should make this trivial to reproduce.

The solution would likely be to store "pending" edit events, when the original event hasn't been seen yet, just like we're doing for pending reactions. A generalization of this fix would be to do that for any kind of events that "modifies" the state of another event: reaction, edit, redaction, poll response (which also have some kind of pending cache in the timeline code).

zecakeh commented 2 months ago

The reason that we drop edits when back-paginating is that we rely on the server to bundle (aka aggregate) the edit on the original event, so we should still get the edit when we receive the original event. Same goes for redactions. Reactions have a pending cache for that.

The reason this happened in encrypted rooms was that the bundled edit was never decrypted with the original event, so the code couldn't apply an encrypted edit to a decrypted message. Now we do decrypt bundled events with original events so issues like this and #2848 should be mostly fixed.

bnjbvr commented 2 months ago

Ah, I see, thanks. I suppose we should write a regression test that failed before the patch, and since it involves encryption, it might not be trivial to write… Could be an fully-blown integration test, maybe?

Also: what do you mean by "mostly" fixed? :sweat_smile:

zecakeh commented 2 months ago

Ah, I see, thanks. I suppose we should write a regression test that failed before the patch, and since it involves encryption, it might not be trivial to write… Could be an fully-blown integration test, maybe?

That's already what you asked in #2848, right?

Also: what do you mean by "mostly" fixed? 😅

If we don't have the encryption key of the edit when we decrypt the original event, the same problem will appear, and we don't retry to decrypt the bundled event when we receive new encryption keys. This brings the question of whether we should change the event to an UnableToDecrypt if the edit is not decrypted but the original event is. It is more correct to show that the current state is wrong, but the users will have more info with the original event.

It is a problem that will occur way less often, and will probably be solved when the timeline is reloaded, but still a problem.

However it is the same problem as if the edit is redacted after we display the event. We don't update the event to revert it back to the original (or the previous edit). Basically we don't keep track currently of what happens to the related event on the original event. I am pretty sure this is (or was?) listed in an issue about the timeline somewhere.

bnjbvr commented 2 months ago

Thanks for the explanation about "mostly", it makes sense now. I'll open an issue about retrying decryptions, since this is going to move out of the timeline code as per the event cache issue.

That's already what you asked in https://github.com/matrix-org/matrix-rust-sdk/issues/2848, right?

I suppose so! I hadn't realized how those two were connected.

bnjbvr commented 3 weeks ago

Most issues should have been fixed thanks to the following PRs:

I've also opened #4106 as a followup for the issue discussed above, which I think would happen fairly infrequently.

Since there are so many different ways (e.g. an edit event was UTD) to run into these symptoms (i.e. an edit doesn't show up), and a lot of engineering effort has been spent fixing the most blatant issues, I would like to close this one. Of course, we're happy to investigate further issues, would they unfortunately happen again. In this case, please provide precise steps to reproduce the problem, or rageshakes at the very least (with a log level of trace enabled for the timeline). Thanks all!