Notifications can leave a ghostly heading-notification behind #5119

gnprice · 2021-11-10T03:07:57Z

This is an issue that doesn't exist in any released version; it was introduced with the spiffy new form of notification in #4842, and I discovered it later yesterday while preparing some refactors to that code after merge.

The repro recipe is:

Have notifications active for several conversations in the same account; e.g., have another user send PMs in two different threads.
Go read all those conversations at the same time, e.g. by going to the "Private messages" view in the web app.
Expected: The notifications disappear.
Actual: The notifications for the individual conversations disappear. But an empty shell of a header notification remains. It looks like this screenshot:

(which was for a previous bug, fixed before merge, that once it was triggered had the same symptom.)

The root of the issue is that in the new removeNotification we're relying on an assumption that the loop looking for notifications to cancel found exactly one such notification. If it found more than that, the code won't realize if the summary notification should also be cancelled.

The text was updated successfully, but these errors were encountered:

If the user reads the last messages in several conversations at once, and those are their only active notifications -- for example, if they have a couple of recent PMs in different threads, and visit the "Private messages" view in the web app -- then it looks from the code here like we'll delete those individual conversations' notifications, but not the summary for the group, leaving behind a ghost header. I noticed this just now from reading the code. I haven't confirmed empirically that this indeed happens. If there's a subtle reason it doesn't, though, we should resolve this TODO by making that clear. [Added later: Indeed, confirmed this empirically; filed as zulip#5119.]

I added this TODO comment a few commits ago, based on studying the code, and just now I confirmed that the bug happens empirically: reading several conversations at once can leave behind a ghost group. There's also a related bug when the user reads a conversation whose notification has already been dismissed: if there's just one active notification remaining, we'll end up dismissing the whole group and that notification with it. The root cause of both of these is that we're relying on an assumption that the loop looking for notifications to cancel found exactly one such notification. In fact it may have found fewer or more than that. This can cause this code to leave the summary notification in place when it should cancel it, and vice versa. To fix, while we're looping through the notifications in order to possibly cancel some, we just note whether there are also any (in the same group) that we aren't cancelling. Fixes: zulip#5119 Fixes: zulip#5120

If the user reads the last messages in several conversations at once, and those are their only active notifications -- for example, if they have a couple of recent PMs in different threads, and visit the "Private messages" view in the web app -- then it looks from the code here like we'll delete those individual conversations' notifications, but not the summary for the group, leaving behind a ghost header. I noticed this just now from reading the code. I haven't confirmed empirically that this indeed happens. If there's a subtle reason it doesn't, though, we should resolve this TODO by making that clear. [Added later: Indeed, confirmed this empirically; filed as zulip#5119.]

I added this TODO comment a few commits ago, based on studying the code, and just now I confirmed that the bug happens empirically: reading several conversations at once can leave behind a ghost group. There's also a related bug when the user reads a conversation whose notification has already been dismissed: if there's just one active notification remaining, we'll end up dismissing the whole group and that notification with it. The root cause of both of these is that we're relying on an assumption that the loop looking for notifications to cancel found exactly one such notification. In fact it may have found fewer or more than that. This can cause this code to leave the summary notification in place when it should cancel it, and vice versa. To fix, while we're looping through the notifications in order to possibly cancel some, we just note whether there are also any (in the same group) that we aren't cancelling. Fixes: zulip#5119 Fixes: zulip#5120

If the user reads the last messages in several conversations at once, and those are their only active notifications -- for example, if they have a couple of recent PMs in different threads, and visit the "Private messages" view in the web app -- then it looks from the code here like we'll delete those individual conversations' notifications, but not the summary for the group, leaving behind a ghost header. I noticed this just now from reading the code. I haven't confirmed empirically that this indeed happens. If there's a subtle reason it doesn't, though, we should resolve this TODO by making that clear. [Added later: Indeed, confirmed this empirically; filed as zulip#5119.]

I added this TODO comment a few commits ago, based on studying the code, and just now I confirmed that the bug happens empirically: reading several conversations at once can leave behind a ghost group. There's also a related bug when the user reads a conversation whose notification has already been dismissed: if there's just one active notification remaining, we'll end up dismissing the whole group and that notification with it. The root cause of both of these is that we're relying on an assumption that the loop looking for notifications to cancel found exactly one such notification. In fact it may have found fewer or more than that. This can cause this code to leave the summary notification in place when it should cancel it, and vice versa. To fix, while we're looping through the notifications in order to possibly cancel some, we just note whether there are also any (in the same group) that we aren't cancelling. Fixes: zulip#5119 Fixes: zulip#5120

If the user reads the last messages in several conversations at once, and those are their only active notifications -- for example, if they have a couple of recent PMs in different threads, and visit the "Private messages" view in the web app -- then it looks from the code here like we'll delete those individual conversations' notifications, but not the summary for the group, leaving behind a ghost header. I noticed this just now from reading the code. I haven't confirmed empirically that this indeed happens. If there's a subtle reason it doesn't, though, we should resolve this TODO by making that clear. [Added later: Indeed, confirmed this empirically; filed as zulip#5119.]

I added this TODO comment a few commits ago, based on studying the code, and just now I confirmed that the bug happens empirically: reading several conversations at once can leave behind a ghost group. There's also a related bug when the user reads a conversation whose notification has already been dismissed: if there's just one active notification remaining, we'll end up dismissing the whole group and that notification with it. The root cause of both of these is that we're relying on an assumption that the loop looking for notifications to cancel found exactly one such notification. In fact it may have found fewer or more than that. This can cause this code to leave the summary notification in place when it should cancel it, and vice versa. To fix, while we're looping through the notifications in order to possibly cancel some, we just note whether there are also any (in the same group) that we aren't cancelling. Fixes: zulip#5119 Fixes: zulip#5120

gnprice added a-Android a-notifications P1 high-priority labels Nov 10, 2021

gnprice self-assigned this Nov 10, 2021

gnprice mentioned this issue Nov 10, 2021

Reading one conversation can cause another one's notification to disappear #5120

Closed

gnprice mentioned this issue Nov 10, 2021

Revise Android notifs code after recent overhaul #5121

Merged

gnprice closed this as completed in 2e778ec Nov 16, 2021

gnprice mentioned this issue Jun 18, 2024

notif: Create summary notification zulip/zulip-flutter#703

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Notifications can leave a ghostly heading-notification behind #5119

Notifications can leave a ghostly heading-notification behind #5119

gnprice commented Nov 10, 2021

Notifications can leave a ghostly heading-notification behind #5119

Notifications can leave a ghostly heading-notification behind #5119

Comments

gnprice commented Nov 10, 2021