-
-
Notifications
You must be signed in to change notification settings - Fork 651
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
Never get notifications (when recently used webapp, or app is in background) #3659
Comments
N.B.: This may be related to a note on the Android Pie changelog describing how notifications from "suspended" apps have hitherto been canceled. If so, I'd expect that on Pie devices the user will receive one notification containing both the step 4 and step 6 PMs. (In that case we'll probably still want some sort of workaround for pre-Pie systems, though.) |
Thanks for this report! I didn't repro this on my phone (Pixel 2 XL, running Android 10 and the current Zulip beta) when we tried earlier today. I'm not sure if we satisfied this bit, though:
How quickly does it seem to need to be to trigger the issue? |
I don't recall if you had the PMing user's message-list open, though – I think you might have been at the PMs-list page. (For reference, I tested on a Samsung with Marshmallow. The emulator had similar issues, but I've found in-emulator notifications to be of questionable reliability anyway.)
I haven't tested this extensively; I've just tried to keep it down to 5s or so. |
Hmm, yeah. I just tried again making sure of that step too, though -- so it was steps 1-4 as written above exactly -- and it still didn't reproduce; the notification arrived promptly as usual. (Same Android 10 device as mentioned above; the app version is 26.11.134.) I demonstrated for you in person just now, and you added that it reliably reproduces on that Samsung Galaxy J5 running Android 6 Marshmallow. More investigation to be done, then 🙂 This is quite a bad symptom, so if this shows up on a lot of people's devices (and so far it's hard to say whether it's rare or very common), then it's an important one to figure out and fix. |
The notifications are not being displayed because the server is not sending them – and this appears to be documented behavior. Speaking strictly from a user perspective, this was confusing! Off the cuff, I suspect a better approach might be to allow the app to jump the gun, and immediately set itself as "idle" for the purposes of notifications when it loses the foreground state. |
Hmm! I guess it does say:
That doesn't seem like great behavior, thinking about it when put that way.
In fact, I think this is very likely to be the cause of a solid fraction of the reports we sometimes get that "notifications don't work". (... I'm going to go turn off that setting (reverting to the default behavior) for my own czo account. I'd forgotten I even had it on.) |
Yeah, the tricky thing about this "offline or note" thing is that the simple alternative is that if you're on your computer having a long PM conversation with someone, then every time you they send you a message, your phone in your pocket vibrates and dings, which can be a quite annoying user experience. It's probably a good idea at the very least to flip the user-level default behavior (so that you get the potentially-annoying-but-it-clearly-works experience at first, and can turn it off, rather than the alternative). But currently the time threshold is 80-140s (the range because of every-minute-syncing of presence data); if you haven't done anything in your Zulip browser tab for 140s, it switches to dinging your phone. Now that we have actual code that runs on the phone at the time a message is received that can control behavior, we can also consider fancier options for what to do in the "maybe don't ding my phone on every PM because I'm actively using Zulip on my computer" case (like making the notifications appear in the phone's notification area, but not making a ding sound, rather than not sending a notification at all, so you can see it right there on your phone if you check). |
@ray-kraesig Could this explain the behavior you saw?
Hmm. Yeah, I agree that behavior isn't awesome in that case. That description brings to mind a comparison data point, though: I'm pretty sure that is exactly the behavior I routinely get with both Facebook Messenger and Google's "Messages". (The latter being the SMS client on stock Android in recent years, and also https://messages.google.com/web .)
Yeah, I think this is probably right. |
I think those 1:1 chat apps not used for work aren't the right comparison point in that the tradeoffs are different; if the use case for the app is 90% mobile, most activity will be on mobile and the consideration of what to do when the person is at their desk on their computer isn't particularly important. I would look at apps like Slack or Discord that are more about large group conversations to see how other apps address this. |
(I've edited the title, labels, and top of the description to reflect where this thread went once we understood the cause better.) |
Just this afternoon I had an alarming experience that it took me a moment to realize was because of this behavior. I actually had the thought "oh no, notifications aren't working"; and if I hadn't specifically known about this behavior, I would have taken that away as my conclusion. Specifically, I was setting up a new personal phone. Among the many apps to go open up, log into, and make sure everything was set up in was Zulip. So I opened it up, logged in... and then I wanted to check that notifications worked. So I returned to the launcher, then on my desktop opened up an alternate browser profile and logged into a demo user, and sent myself a PM from there. (If I weren't a Zulip developer, I'd have gotten the same effect by asking a colleague to send me a PM.) Nothing happened. 😮 I waited for a bit -- still nothing. Oh no! What was wrong? Then I remembered this thread. In the webapp I went and switched to the "always send" setting, and repeated the test. Perfect success -- the notification arrived immediately. Before the discussion above, I'd had the "always send" setting enabled for probably close to two years, and it's never bothered me. I've been on the default, "avoid sending" setting for less than two weeks (*) before hitting this experience. We can iterate on refining the heuristics that drive it, but I don't think they're currently reliable enough to be an acceptable default for users, and we should switch the default to "always send". (*) since Nov 11, I think; I mentioned doing so in an earlier comment, but I forgot then and remembered when coming back to this thread. |
Some further observations on the current behavior of the "avoid sending" heuristics:
(ongoing...) |
I can confirm that this does not require any use of the webapp; I've just tested with a user who hasn't, AFAIK, logged in anywhere but the mobile app for the last several hours. |
Now past 3 minutes, and no notification. So it looks like both the webapp and the mobile app can (with the heuristics that currently are the default setting) prevent notifications from getting sent, even when they're away in the background. |
It looks like on the webapp side the behavior is basically
Further observations were:
|
Well, this is different. I recently sent four PMs to the Zulip test user: two before killing the app, two after. As expected (given all the other diagnosis), I received the latter two. Quite unexpectedly, however, I then also received notifications for the former two a few minutes afterward – about ten minutes after the PMs were first sent. It may be relevant that I had two devices active. |
I haven't had a chance to read the full thread here, but I can explain that 10 minutes thing at least. The backend logic for whether we consider the receiver user "off zulip" has a few parts:
So the 10 minutes after case is almost certainly that 3rd code path. I think there's a couple possible bugs here:
|
Very informative, thanks! In addition to the case Ray observed where he got notifications about 10m after the messages were sent, one or two of my test cases described above were well over 10 minutes, and in none of the cases did I get a belated notification. (There were the couple of cases where there was a notification immediately, and in all other cases I never got a notification.) In those cases, I did have the webapp open in a desktop browser tab, just one that never got focus for a long time. So there would have been an event queue that was actively being consumed. If I understand right:
This only affects messages that are still in the queue, right? In which case it's consistent with the observations where I never got a notification even after more than 10 minutes. More generally, this case will send a notification if the recipient's laptop was already suspended when the message was sent, but not if they were in the process of closing the lid and suspending it, or if they were simply AFK. |
These both sound like good changes, and I think they would resolve the cases Ray described. IIUC they wouldn't have had any effect on some of the cases I described, including the one that alarmed me yesterday and brought me back to this thread. In all of those cases, I'd recently used the webapp and it was still in a tab. So the webapp had me not yet idle, it still had an event queue, and it was actively pulling events off the queue... it's just that I wasn't actively looking at it. One solution for that might look like: in any case where we hold back from sending a mobile notification because we think you're active, we test that empirically by looking N seconds later (like 5-10s) to see if the message is now read. If it's still unread, then you weren't so actively reading there after all, and we go ahead and send the notification. I think a failsafe like that might give the "avoid sending" configuration reliable enough delivery to be acceptable as the default. |
This sounds like it would give a false positive if you're active in the webapp, but currently working in a narrow that excludes the new message. |
There's the rub. I'm coming to think that the mobile app should count as "presence" for the purpose of sending email notifications, but not for the purpose of sending mobile notifications. Or at least that the mobile app should be better about declaring its lack of presence. (We could possibly put something appropriate in Whether and how the webapp should count as "presence" for mobile notifications is the other part of this bug – and one that I think that probably deserves to be considered as a separate issue, instead of being folded in with this one. |
It would mean you get a push notification in that case, yes. Whether that's "false" is more a matter of taste, and of tradeoffs. It's possible there should be an opt-in setting that has more aggressive heuristics to decide you're active and suppress notifications, more like the status-quo default, for people who prefer that tradeoff. What I want to avoid by default is the situation where we suppress a notification because we think you're active, when the reality is that you're paying attention to something else on your computer. That includes cases where Zulip is on your screen -- and then short of eye-tracking it's impossible to distinguish between staring at Zulip and pondering some other messages there, and staring at something else entirely. (Alternatively if we wanted to use "on your screen" as a criterion, I'm not sure we could reliably distinguish it from being in a non-selected tab, or in a window that's on another workspace.) So anything that suppresses notifications in the former case will unavoidably do so in the latter. I guess if we wanted to use the criterion "you're actively giving input to Zulip", we could in principle do that. But that'd be a new stream of data to build. |
Hmm. There is indeed that: handleAppStateChange = (state: 'active' | 'background' | 'inactive') => {
const { dispatch, unreadCount } = this.props;
dispatch(reportPresence(state === 'active')); But then there's also this: export const doInitialFetch = () => async (dispatch: Dispatch, getState: GetState) => {
dispatch(reportPresence());
setInterval(() => dispatch(reportPresence()), 60 * 1000); That doesn't know anything about being in the background or not. And the
So that, at least, seems like straightforwardly a bug. |
Forked that off as #3699 . |
…ponent [This change has not been tested on iOS. Also, there's logging code still in it.] Centralize all presence-reporting logic into a new zero-display React component, currently (and somewhat arbitrarily) located under AppStateHandlers. This fixes at least two (unreported) issues: * If the user logs in and out, or otherwise performs an additional "initial fetch" without killing the app, the presence timer will be started multiple times. (This was previously concealed by throttling logic in usersActions.reportPresence, which was itself removed in a previous patch in this set.) * If the user goes idle (e.g. by hitting the Home button), one `idle` message will be sent... followed immediately by more `active` messages on the original timer. (Whether the latter messages are actually sent may be OS and OS-version dependent.) This is work towards a fix for zulip#3659 and zulip#3699. Unfortunately, there is no user-visible change as yet; modifications will be needed on the Zulip server side as well.
Centralize all presence-reporting logic into a new zero-display React component, currently (and somewhat arbitrarily) located under AppStateHandlers. This fixes at least two issues: * If the user logs in and out, or otherwise performs an additional "initial fetch" without killing the app, the presence timer will be started multiple times. (This was previously concealed by throttling logic in usersActions.reportPresence, which was itself removed in a previous patch in this set.) * If the user goes idle (e.g. by hitting the Home button), the presence timer will still run, and will still attempt to send `active` notifications. (These attempts are often suppressed while the app is in the background, but there's no guarantee of this.) Fixes zulip#3699 (properly), and is work towards zulip#3659. Unfortunately, there is no user-visible change yet: modifications will be needed on the Zulip server side as well.
Centralize all presence-reporting logic into a new zero-display React component, currently (and somewhat arbitrarily) located under AppStateHandlers. This fixes at least two issues: * If the user logs in and out, or otherwise performs an additional "initial fetch" without killing the app, the presence timer will be started multiple times. (This was previously concealed by throttling logic in usersActions.reportPresence, which was itself removed in a previous patch in this set.) * If the user goes idle (e.g. by hitting the Home button), the presence timer will still run, and will still attempt to send `active` notifications. (These attempts are often suppressed while the app is in the background, but there's no guarantee of this.) Fixes zulip#3699 (properly), and is work towards zulip#3659. Unfortunately, there is no user-visible change yet: modifications will be needed on the Zulip server side as well.
Centralize all presence-reporting logic into a new zero-display React component, currently (and somewhat arbitrarily) located under AppStateHandlers. This fixes at least two issues: * If the user logs in and out, or otherwise performs an additional "initial fetch" without killing the app, the presence timer will be started multiple times. (This was previously concealed by throttling logic in usersActions.reportPresence, which was itself removed in a previous patch in this set.) * If the user goes idle (e.g. by hitting the Home button), the presence timer will still run, and will still attempt to send `active` notifications. (These attempts are often suppressed while the app is in the background, but there's no guarantee of this.) Fixes zulip#3699 (properly), and is work towards zulip#3659. Unfortunately, there is no user-visible change yet: modifications will be needed on the Zulip server side as well.
Centralize all presence-reporting logic into a new zero-display React component, currently (and somewhat arbitrarily) located under AppStateHandlers. This fixes at least two issues: * If the user logs in and out, or otherwise performs an additional "initial fetch" without killing the app, the presence timer will be started multiple times. (This was previously concealed by throttling logic in usersActions.reportPresence, which was itself removed in a previous patch in this set.) * If the user goes idle (e.g. by hitting the Home button), the presence timer will still run, and will still attempt to send `active` notifications. (These attempts are often suppressed while the app is in the background, but there's no guarantee of this.) Fixes zulip#3699 (properly), and is work towards zulip#3659. Unfortunately, there is no user-visible change yet: modifications will be needed on the Zulip server side as well.
Centralize all presence-reporting logic into a new zero-display React component, currently (and somewhat arbitrarily) located under AppStateHandlers. This fixes at least two issues: * If the user logs in and out, or otherwise performs an additional "initial fetch" without killing the app, the presence timer will be started multiple times. (This was previously concealed by throttling logic in usersActions.reportPresence, which was itself removed in a previous patch in this set.) * If the user goes idle (e.g. by hitting the Home button), the presence timer will still run, and will still attempt to send `active` notifications. (These attempts are often suppressed while the app is in the background, but there's no guarantee of this.) Fixes zulip#3699 (properly), and is work towards zulip#3659. Unfortunately, there is no user-visible change yet: modifications will be needed on the Zulip server side as well.
Replace the existing presence-reporting logic with HeartbeatComponent, instantiated as part of a new zero-display React component located under AppEventHandlers. In particular, strip the throttling logic from `reportPresence`. This fixes at least three issues: * If the user logged in and out, or otherwise performed an additional "initial fetch" without killing the app, the presence timer would be started multiple times. (The negative effects of this were previously concealed by the throttling logic.) * `idle` events were not treated differently from `active` events by the throttling logic, and would almost invariably be swallowed. * If the user went idle (e.g. by hitting the Home button), the presence timer would still run, and would still attempt to send `active` notifications. (These attempts were often suppressed while the app was in the background, but this was never guaranteed.) Fixes (the original version of) zulip#3699, and is work towards zulip#3659. Unfortunately, there is no user-visible change yet: modifications will be needed on the Zulip server side as well.
Replace the existing presence-reporting logic with HeartbeatComponent, instantiated as part of a new zero-display React component located under AppEventHandlers. In particular, strip the throttling logic from `reportPresence`. This fixes at least three issues: * If the user logged in and out, or otherwise performed an additional "initial fetch" without killing the app, the presence timer would be started multiple times. (The negative effects of this were previously concealed by the throttling logic.) * `idle` events were not treated differently from `active` events by the throttling logic, and would almost invariably be swallowed. * If the user went idle (e.g. by hitting the Home button), the presence timer would still run, and would still attempt to send `active` notifications. (These attempts were often suppressed while the app was in the background, but this was never guaranteed.) Fixes (the original version of) zulip#3699, and is work towards zulip#3659. Unfortunately, there is no user-visible change yet: modifications will be needed on the Zulip server side as well.
Replace the existing presence-reporting logic with HeartbeatComponent, instantiated as part of a new zero-display React component located under AppEventHandlers. In particular, strip the throttling logic from `reportPresence`. This fixes at least three issues: * If the user logged in and out, or otherwise performed an additional "initial fetch" without killing the app, the presence timer would be started multiple times. (The negative effects of this were previously concealed by the throttling logic.) * `idle` events were not treated differently from `active` events by the throttling logic, and would almost invariably be swallowed. * If the user went idle (e.g. by hitting the Home button), the presence timer would still run, and would still attempt to send `active` notifications. (These attempts were often suppressed while the app was in the background, but this was never guaranteed.) Fixes the original version of zulip#3699; the next commit will complete the fix for that issue. This is also work towards zulip#3659. Unfortunately, there is no user-visible change yet: modifications will be needed on the Zulip server side as well.
And with #3727 merged, #3699 is now fixed! There were several angles to this issue, and I'm not sure we 100% got to having a handle on them and I definitely don't have them all in my head now. It may be productive to come back to this issue and think through it again, with the particular confusions that went into #3699 now cleared out of the way. |
We had a report today in chat of symptoms that fall under this issue:
A couple of interesting points there:
EDIT: Aha, this turned out to be a case of the old "some device vendors break notifications to juice battery life" -- where their approach is so broken that they have to exempt some popular apps, to stop their customers from immediately realizing it's the device's fault. We've known about this for a couple of years, but had the information only in chat and support threads; I've now filed #4074 to better track it. |
I've just filed zulip/zulip#18072 in the server repo with a fresh repro of this issue, in the version involving no interaction from the mobile app at all (so there's no question about how the mobile app is reporting presence or anything like that.) I believe that covers the aspects of this that are still open. There may of course still be bugs in the app's behavior -- we should file those as new issues if we find them. Because the issue is experienced as part of the mobile app, I think it's helpful to keep it open in our tracker so it's visible when looking for known issues. I've applied the "dupe from server" label to indicate that's the story. |
EDIT by @gnprice following this comment: the root cause seems to be
Repro procedure:
1. Log in to the Android app as User A.
2. Navigate to the "PMs with User B" screen.
3. Press the Home button to go directly to the Home screen. Do not force-quit the app; at this stage it should still be running in the background.
4. As User B, quickly send a PM to User A.
No notification will appear on the Android device.
5. Bring up the app list. Slide Zulip off into oblivion.
6. As User B, send a PM to User A.
Exactly one notification will (eventually) appear on the Android device, for only the PM sent in step 6.
Expected behavior:
A notification for the PM sent in step 4 eventually appears – either coincident with or prior to the notification for the PM sent in step 6.
The text was updated successfully, but these errors were encountered: