-
Notifications
You must be signed in to change notification settings - Fork 371
fix(latest_event): handle edits when the target event is not directly preceding #6096
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
… preceding Signed-off-by: Johannes Marbach <[email protected]>
f195f1d to
1df3ee1
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6096 +/- ##
==========================================
+ Coverage 89.84% 89.86% +0.02%
==========================================
Files 362 362
Lines 100433 100680 +247
Branches 100433 100680 +247
==========================================
+ Hits 90230 90479 +249
+ Misses 6678 6676 -2
Partials 3525 3525 ☔ View full report in Codecov by Sentry. |
CodSpeed Performance ReportMerging this PR will not alter performanceComparing Summary
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution.
Sorry if the documentation didn't convey this constraint correctly, but there is something important to know.
Why do we only support edits that apply to the immediately previous event? To avoid a situation where an edit of an old message would make the event to pop as a latest event. Imagine you've messages A, B, C.
- The latest event is C.
- Then, C is edited to D. The latest event must be D.
- Then, A is edited to X. You don't want X to be the latest event, you still want D to be The One.
Now. I understand the problem with a double edit. Sorry for having missed this… and thanks for tackling this problem.
But, we need to be very aware of the rule: an edit must be ignored if another event is inserted between the edit and the related event. And that's not what your code does as far as I understand it, right?
…irectly preceding Add test case where the latest edit doesn't target the latest event Signed-off-by: Johannes Marbach <[email protected]>
If I understood correctly, it should actually do just that. The new logic is to go backwards, remembering edits until you hit a target event. Then you return the latest edit of that event or the event itself if there are no edits. In your example, the chain is: The code should skip over the two edits until it hits I've added a test case for it in a92ae5a. |
Problem: The latest event calculation currently only considers edits when the replacement event directly follows the replaced event with no other events in-between. This leads to incorrect latest event values in certain situations such as when an event is edited twice in a row.
This PR changes the calculation to remember the latest edit per event ID, continue iterating backwards until an actual matching event is found and then return that event's latest edit or the event itself if it hasn't been edited.