-
Notifications
You must be signed in to change notification settings - Fork 270
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
timeline: only use the send queue mechanism to send reactions #3865
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3865 +/- ##
==========================================
- Coverage 84.11% 84.10% -0.02%
==========================================
Files 266 266
Lines 27787 27724 -63
==========================================
- Hits 23372 23316 -56
+ Misses 4415 4408 -7 ☔ View full report in Codecov by Sentry. |
9128e57
to
b21cf41
Compare
b21cf41
to
2908abe
Compare
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.
I like the way it goes, and it's clearly an improvement. Less code, more re-used code, well done!
I wonder if the patch could be split into multiple smaller ones though, it's not easy to review, but generally the changes look good! I'm confident with the tests.
2908abe
to
f59388e
Compare
f59388e
to
0723d4c
Compare
Independently of #3749, we can, as of today, start using the send queue only to send reactions, instead of having the logic of sending reactions in the timeline itself
There was a mechanism to queue adding/removing a reaction, with the possibility to get observable updates for each add/remove that occurred. In some contexts, if you quickly tapped a reaction button on ElementX apps, you could observe it: the reaction would be added, then removed, then added back, etc. until it stabilized.
I don't see the user value for this: if I quickly tap the reaction button, I'd expect the sending of the reaction to be aborted, if it can, instead of delayed into a redaction event that's subsequently sent. The sending queue allows doing this for free, so making use of that.
I've also removed tests which value was also unclear or suspect.