-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Fix team sidebar not showing thread unreads #8624
base: main
Are you sure you want to change the base?
Conversation
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.
Nice! And great testing work. Only left non-blocking comments.
return channelUnreads.pipe( | ||
combineLatestWith(threadsUnreadsAndMentions), | ||
map$(([channels, threads]) => { | ||
return channels || threads.unreads; |
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.
Could you explain why in one case (channels) we only need to check if the value is defined while for threads we need to check the unreads
property specifically?
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.
Probably a case of bad variable naming.
observeMyChannelUnreads
is observing a boolean value. So the result of that is just true or false (I have unreads in my channels or not).
observeUnreadsAndMentionsInTeam
is observing an object with the unreads and the mention count. In this case, we only want the unreads (we understand a mention count implies an unread).
I am having a hard time thinking of better names, and github copilot doesn't seem to be doing a good job at that 😅 If you have any ideas, I am happy to update that.
switchMap(([mycs, notify]) => of$(mycs.reduce((acc, v) => { | ||
const isMuted = notify?.[v.id]?.mark_unread === 'mention'; | ||
return acc || (v.isUnread && !isMuted); | ||
}, false))), |
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 feel some comments to explain the logic could be beneficial for people that don't usually deal with mobile code, and all the rxjs syntax involved.
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.
Maybe I am falling into a bit of knowledge curse. What is hard to understand here that has to deal with rxjs?
From the rxjs part, we only have the switchMap
and the of$
. Both are a bit alien for anyone that have never used rxjs, but they are all over the code, so it doesn't make sense to document them (I think).
The rest of the code, is a simple list reduce, that checks the business logic of mattermost around when a message is considered unread (is actually unread, and is not muted).
So... not sure if I am missing something.
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.
LGTM 🚀 , Awesome work.
Summary
The team sidebar was not listening to unreads on threads. This didn't look as a regression, but something missed when migrating to Gekidou.
Ticket Link
Fix https://mattermost.atlassian.net/browse/MM-63202
Release Note