Skip to content
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

"N unreads" banner on "All messages", but no unreads in home view #3589

Open
gnprice opened this issue Aug 14, 2019 · 3 comments
Open

"N unreads" banner on "All messages", but no unreads in home view #3589

gnprice opened this issue Aug 14, 2019 · 3 comments
Labels
a-data-sync Zulip's event system, event queues, staleness/liveness a-message list help wanted

Comments

@gnprice
Copy link
Member

gnprice commented Aug 14, 2019

Originally reported in chat:
https://chat.zulip.org/#narrow/stream/48-mobile/topic/all.20messages.20and.20unread.20messages/near/779399

Screenshots there show

  • the app's home tab with "No unread messages", and no counters on PMs or mentions etc.
  • the "All messages" screen, with a "4 unread messages" banner. None of the messages in view have a green unread-marker.

So there's a mismatch between two different presentations of whether there are unreads.


This is a bit like #3549. There:

  • the green marker clears (not clear if that's happening or not in this case)
  • but the "unread messages" banner at the top sticks around
  • ... but in that issue, the message remains in the home view!

In that issue, I wrote:

we have two conflicting ideas of whether each message is read, causing us to show the inconsistencies Rishi described. The Redux store controls the home view and the "1 unread message" banner, and the data inside the webview controls the unread marker. I believe the unread flag in the Redux store only gets cleared when we hear back from the server.

But the inconsistency seen in this issue is between those two things that are controlled by the Redux store.

Some hypotheses on where the bug could be:

  • There are a couple of different spots in the Redux store that have relevant information -- I think the banner is based on the per-message read flags under state.flags.read, while the home view is based on some summary data structures in state.unread. So if there's a bug where the reducers managing those two subtrees of the state get the data out of sync, that could cause these symptoms.

  • It could also be that that code is working perfectly fine and that data is staying in sync, and there's an issue elsewhere. For example, maybe the "All messages" view includes some messages that don't appear in the home screen's unreads. Possible hypotheses for that include: muted topics? muted streams? @-mentions? alert words?

    Probably we ought to have an invariant that the messages that count for the unreads list in the home view are exactly the same as the messages that show up in "All messages". But I'd have to dig into the code to be confident about whether we do have such an invariant.

Possibly also related to #3509. It's not clear in that issue whether the home tab with its unreads list is in sync with the "N unreads" banner.

@gnprice gnprice added a-message list a-data-sync Zulip's event system, event queues, staleness/liveness labels Aug 14, 2019
@andersk
Copy link
Member

andersk commented Nov 27, 2019

This happens to me all the time. I just confirmed that the “All messages” unread counter is counting messages on a muted stream (I have #test here muted), even though the messages aren’t shown in either the home view or the “All messages” view.

gnprice added a commit that referenced this issue Apr 15, 2020
Also several TODO comments, where it looks like the actual semantics
and intended semantics differ.

One of those looks to be the cause of a bug we've gotten reports of!
Namely #3589.  So, hooray for closely reading code.
@gnprice
Copy link
Member Author

gnprice commented Apr 15, 2020

In the course of studying some of our code closely for the sake of #4035, I think I've spotted the cause of this bug!

It's in our getUnreadTotal selector, which controls the "N unread messages" counter in the case of the "All messages" view. Here's the comments I just added in f7f2fc6:

/** Total number of unreads, with ??? caveats. */
// TODO: This appears to double-count @-mentions.  Why not just use `count`
//   from the `unread_msgs` data structure?
// TODO: This seems to include muted streams, though not muted topics;
//   see `getUnreadStreamTotal`.  Looks like this causes #3589.
export const getUnreadTotal: Selector<number> = createSelector(

A direct fix for this issue would be to fix getUnreadStreamTotal so that it excludes muted streams. We should also drop the use of getUnreadMentionsTotal from getUnreadTotal.

A more thorough fix would be to use the count property of the unread_msgs data structure (see the jsdoc I just added to src/api/initialDataTypes.js. We'd add that to the UnreadState type, and adjust our reducers to maintain it as new unreads arrived and as existing ones got read.

@PoignardAzur
Copy link

This bug still appears on the Android app.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-data-sync Zulip's event system, event queues, staleness/liveness a-message list help wanted
Projects
None yet
Development

No branches or pull requests

3 participants