msglist: Update when messages marked unread #5537
Conversation
gnprice
added
a-message list
P1 high-priority
server release goal
gnprice
force-pushed
the
pr-unread-inbound-event
branch
from
c230ada to
c0a8f56
Compare
| * The two returned arrays consist of items in only the first array, and | ||
| * items in only the second array, respectively. | ||
| * | ||
| * Both input arrays must be sorted. The output arrays will also be sorted. |
There was a problem hiding this comment.
I think the requirement is specifically that they're sorted ascending, is that right?
In the symmetricDiff caller you add in this PR, it's pretty easy to see that both input arrays are sorted ascending:
prevReadIds.sort((a, b) => a - b);
nextReadIds.sort((a, b) => a - b);
const [newlyUnread, newlyRead] = symmetricDiff(prevReadIds, nextReadIds);(Well, I did a quick test in Chrome DevTools console, since I haven't memorized whether ascending is a - b or b - a.)
But do you think future callers might not be as well-behaved, or prone to change so they violate the invariant? What do you think about a dev-only error?
There was a problem hiding this comment.
I think the requirement is specifically that they're sorted ascending, is that right?
Yep. That's what "sorted" conventionally means when not otherwise specified.
In our codebase, searching for "sorted" finds a number of other places we say things are "sorted" (or "sorted by" some key, etc.), and they have the same meaning.
But do you think future callers might not be as well-behaved, or prone to change so they violate the invariant? What do you think about a dev-only error?
Good question. I think that's possible but not an especially high risk, and this isn't a case where we need such a debug check.
|
Thanks for the review! I have indeed manually tested the change -- reproduced #5536 before the fix, then checked out the fix and it no longer reproduced. So I'll go ahead and merge. |
Not sure how it got this way in the first place, when this file was added in 82e6db3. Perhaps I let the IDE auto-add the import, it went for this style, and I didn't notice?
Hooray for unit tests -- these actually caught a silly bug in my original draft. I'd even done some manual testing of a new feature in a draft implementation built on this, and that just happened to avoid triggering the bug.
We'll start actually producing such events in a separate commit.
This fixes a bug: if you're looking at a message list in the mobile app and then mark some of those messages as unread (from another client), they wouldn't regain their unread-markers. The "N unread messages" banner at the top *would* update, but the individual messages would still appear to be read. Effectively this was a case where the props change in a way that should describe a change in what's shown; we're preventing React from doing its job to update things, taking responsibility for doing it ourselves through the inbound-events mechanism; and we had a gap in this code for doing that. The old logic was correct in the past, when it was impossible for a read message to become unread. So from another angle, the root cause here is that this is one of the many places across the Zulip project where that longstanding invariant had been embedded as an assumption, and so needed revision when we started breaking that former invariant. Fixes: zulip#5536
gnprice
force-pushed
the
pr-unread-inbound-event
branch
from
c0a8f56 to
0833ea7
Compare
2 participants
Ran into this while implementing #5364. But it also fixes an already-live bug, for when a user marks messages unread from a different client while they have a message list open.
Fixes: #5536