"No messages" on following a notification #3284
Labels
a-data-sync
Zulip's event system, event queues, staleness/liveness
a-notifications
P0 critical
Highest priority
Comments
gnprice
added
a-notifications
a-data-sync
Closed
gnprice
added a commit
that referenced
this issue
Jan 16, 2019
Fixes #3284. In commit 20.0.103~40 aka 1d05ecd "fetch: Remove unused message fetch code in fetchEssentialInitialData", we removed some old code that had been intended to fetch the messages found in the current narrow. We reasoned at the time that (a) this code no longer ever did anything, because `getTopMostNarrow` would always return `undefined`, because we don't persist the nav state (as we used to long ago); (b) in the one case where we don't start on the main nav screen when a logged-in user starts the app -- namely a notification -- we did that via a `doNarrow`, which takes care of the fetch. So this code's job was already being done a different way. In fact, it turns out this code was filling an essential role in making the experience of "start Zulip by following a notification" work at all. Even though its intended design no longer made sense! On (a): this code runs in the initial fetch, which is triggered *after* rehydrate; and *during* rehydrate, `navReducers` takes us to the narrow for the initial notification, if there is one. So there is a topmost narrow here in that case. On (b): the `doNarrow` in question happens very early: dispatched from `handlePendingNotifications`, called from `handleInitialNotification`, which is called at the top of the `componentDidMount` of `AppEventHandlers`. At this stage, it's likely (perhaps certain) we *haven't* rehydrated yet... and `doNarrow` has a conditional to bail out and do nothing if we aren't rehydrated. As a result, removing this meant that tapping a notification, if Zulip wasn't already running, showed a "No messages" screen. Ouch. As a minimal fix for now, put this logic back. The new version is a bit shorter than the old, using the `fetchMessagesInNarrow` helper to conform to surrounding code in the current codebase. A more literal rendition of the old version didn't work; I didn't fully debug, in the interest of getting this out. This code still isn't the right design for handling this; I wouldn't merge something like this if it didn't fix an urgent critical bug. We'll want to sort this logic out better sometime soon.
|
I am re-opening this as is still partially true with the reversion of the commits. |
|
Thanks @borisyankov for that report -- I just filed #3319 as a separate issue for it, because as you say it's a more minor problem and I'd prefer not to complicate the history of this P0 issue 🙂 . |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
This appears to happen 100% of the time on 22.0.107 if:
This is the version that's currently the beta on iOS, and went to prod yesterday on Android.
(I think if it's a conversation we have seen, we'll show a stale list of messages, i.e. omitting the new one that actually caused the notification. Also bad! Not sure of that, though.)
Report here, and extensive debugging discussion below:
https://chat.zulip.org/#narrow/stream/48-mobile/topic/notifications/near/682058
The text was updated successfully, but these errors were encountered: