Fix highlights from threads disappearing on new messages #4106
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This changes interface of Room, so this is a BREAKING CHANGE. Correctly mirrors the logic we use for room notifications for thread notifications, ie. set only the total notifications count from the server if it's zero. I'm not delighted with this since it ends up with function on room whose contract is to do something frankly, deeply weird and unintuitive. However, this is the hack we use for room notifications and it, empirically, works well enough. To do better, we'd need much more complex logic to overlay notification counts for decrypted messages. Fixes element-hq/element-web#25523
florianduros
approved these changes
Mar 12, 2024
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
dbkr
added a commit
that referenced
this pull request
Mar 14, 2024
There's collection of logic for for processing receipts and recomputing notifications for encrypted rooms, but we didn't do the same for threads. As a reasult, when I tried pulling some of the logic over in #4106 clearing notifications on threads just broke. This extends the logic of reprocessing local notifications when a receipt arrives to threads. Based on #4109
4 tasks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue bot
pushed a commit
that referenced
this pull request
Mar 21, 2024
* Move code for processing our own receipts to Room This is some code to process our own receipts and recalculate our notification counts. There was no reason for this to be in client. Room is still rather large, but at least it makes somewhat more sense there. Moving as a refactor before I start work on it. * Add test for the client-side e2e notifications code * Extend logic for local notification processing to threads There's collection of logic for for processing receipts and recomputing notifications for encrypted rooms, but we didn't do the same for threads. As a reasult, when I tried pulling some of the logic over in #4106 clearing notifications on threads just broke. This extends the logic of reprocessing local notifications when a receipt arrives to threads. Based on #4109 * simplify object literal * Add tests & null guard * Remove unused imports * Add another skipped test * Unused import * enable tests * Fix thread support nightmare * Try this way * Unused import * Comment the bear trap * expand comment
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
We were always ignoring the highlight count, even for encrypted rooms, which was broken because we don't do the local calculation for unencrypted rooms.
|
Sorry, this needs another look as I failed to copy the logic correctly. It now does highlights properly (making the behaviour the function even more peculiar). |
florianduros
approved these changes
Mar 21, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This changes interface of Room, so this is a BREAKING CHANGE.
Correctly mirrors the logic we use for room notifications for thread notifications, ie. set only the total notifications count from the server if it's zero.
I'm not delighted with this since it ends up with function on room whose contract is to do something frankly, deeply weird and unintuitive. However, this is the hack we use for room notifications and it, empirically, works well enough. To do better, we'd need much more complex logic to overlay notification counts for decrypted messages. It could be less weird if we made the function able to zero out either type of count with an option, but since we'd only actually use it with one type, this seems worse, not better.
Fixes element-hq/element-web#25523
Checklist
public/exportedsymbols have accurate TSDoc documentation.