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.

Sign up for GitHub

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

Jump to bottom

"No messages" when in fact we're loading messages #3802

Closed
gnprice opened this issue Jan 14, 2020 · 0 comments · Fixed by #3822
Closed

"No messages" when in fact we're loading messages #3802

gnprice opened this issue Jan 14, 2020 · 0 comments · Fixed by #3822
Assignees
@chrisbobbe
Labels
a-data-sync Zulip's event system, event queues, staleness/liveness a-message list P1 high-priority

Comments

@gnprice
Copy link
Member

gnprice commented Jan 14, 2020

This is perhaps the most conspicuous bug I regularly see at present when using the app. I see it on v26.20.143, and I think I've been seeing it on previous versions for weeks at least, perhaps longer.

The typical sequence is:

  • You had the app open, and had been looking at some narrow.
  • You go do something else, and come back to the app after a while -- like over 10 min. (The time for an event queue to expire.)
  • You re-open the app; it was still in the background, so still has its navigation state and shows you that narrow.
  • But for a few seconds, the message list in the narrow says "No messages! Why not start the conversation?", before switching to the "loading" animation.

I think basically what's going on is:

  • We try to poll the event queue, and learn it's expired.
  • OK, so we go and set up a new event queue, which will come with a new snapshot of all the data.
  • Along with that, we have to discard all our existing message data as stale -- any of those messages could have been edited, deleted, had reactions added or removed, etc., since we last polled the queue, and we wouldn't know.
  • But we do that in a way that leaves us acting as if we knew there were no messages in those narrows, etc. This might be as simple as an ordering issue; e.g. perhaps we shouldn't discard the message data at DEAD_QUEUE (when we learn the event queue has expired), but at REALM_INIT (kind of misnamed; dispatched when the new /register completes.)

Some history (which I haven't fully reread and absorbed) for cross-reference:

See also chat discussion. There's an issue affecting MessageReactionList, where state.messages has no message with the expected message ID, which potentially is how the same underlying issue shows up if in step 1 you were looking at a reaction list rather than a message list.
@gnprice gnprice added a-message list a-data-sync Zulip's event system, event queues, staleness/liveness P1 high-priority labels Jan 14, 2020
@chrisbobbe chrisbobbe self-assigned this Jan 14, 2020
This was referenced Jan 15, 2020
Closed
Closed
chrisbobbe pushed a commit to chrisbobbe/zulip-mobile that referenced this issue Jan 17, 2020
redux: Discard messages/narrows on REALM_INIT, not DEAD_QUEUE.
0f14ddb 
Fixes zulip#3802.

When returning to the app after the event queue has expired, there's a moment
after stale data is purged when the app incorrectly believes that an empty
message list is the up-to-date state, and "No messages! Why not start the
conversation?" displays until fresh data arrives.

Currently, state.messages and state.narrows are emptied on the DEAD_QUEUE
action. Instead, we can keep the data around, and clear it in response to
REALM_INIT. At REALM_INIT time, fetchTopMostNarrow, fetchPrivateMessages,
and (sometimes) fetchMessagesInNarrow are called to ensure the stale data
is refreshed. All of these handle the loading state correctly, by doing
MESSAGE_FETCH_START (set loading to true) -> wait for messages ->
MESSAGE_FETCH_COMPLETE (set loading to false).

Clearing the messages in MESSAGE_FETCH_START was considered but
rejected; there are cases (fetchNewer and fetchOlder) when we want to
keep messages around while fetching new ones. Doing it at
INITIAL_FETCH_START was rejected because, like DEAD_QUEUE, it is dispatched
before the /register request. REALM_INIT seems like the logical place
to do it. It is only dispatched from doInitialFetch, so any unintended
consequences should be pretty self-contained.
This was referenced Jan 17, 2020
Merged
Closed
chrisbobbe pushed a commit to chrisbobbe/zulip-mobile that referenced this issue Jan 30, 2020
redux: Discard messages/narrows on REALM_INIT, not DEAD_QUEUE.
894d1a4 
Fixes: zulip#3802.

When returning to the app after the event queue has expired, there's
a moment after stale data is purged when the app incorrectly
believes that an empty message list is the up-to-date state, and "No
messages! Why not start the conversation?" displays until fresh data
arrives.

Currently, state.messages and state.narrows are emptied on the
DEAD_QUEUE action, which is dispatched before the /register request.
Instead, preserve the stale data until /register completes, clearing
it in response to REALM_INIT.

In doInitialFetch, fetchTopMostNarrow, fetchPrivateMessages, and
(sometimes) fetchMessagesInNarrow are also dispatched after
/register to ensure the stale data is refreshed. All of these
already handle their loading states correctly, on a per-narrow basis
in state.fetching.

Clearing the messages in MESSAGE_FETCH_START instead of REALM_INIT
was considered but rejected because MESSAGE_FETCH_START is
dispatched before the /register request.
@gnprice gnprice mentioned this issue Feb 4, 2020
Closed
@gnprice gnprice mentioned this issue Feb 13, 2020
Merged
chrisbobbe pushed a commit to chrisbobbe/zulip-mobile that referenced this issue Feb 19, 2020
redux: Discard messages/narrows/flags/caughtUp on REALM_INIT, not DEA…
0bfc97d 
…D_QUEUE.

Fixes: zulip#3802.

When returning to the app after the event queue has expired, there's
a moment after stale data is purged when the app incorrectly
believes that an empty message list is the up-to-date state, and "No
messages! Why not start the conversation?" displays until fresh data
arrives.

Currently, state.messages, state.narrows, state.flags, and
state.caughtUp are emptied on the DEAD_QUEUE action, which is
dispatched before the /register request. Instead, preserve the stale
data until /register completes, clearing it in response to
REALM_INIT.

In doInitialFetch: fetchTopMostNarrow, fetchPrivateMessages, and
(sometimes) fetchMessagesInNarrow are also dispatched after
/register to ensure the stale data is refreshed. All of these
already handle their loading states correctly, on a per-narrow basis
in state.fetching.

Clearing the data in MESSAGE_FETCH_START instead of REALM_INIT
was considered but rejected because MESSAGE_FETCH_START is
dispatched before the /register request.
@chrisbobbe chrisbobbe mentioned this issue Feb 25, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@chrisbobbe chrisbobbe

Labels
a-data-sync Zulip's event system, event queues, staleness/liveness a-message list P1 high-priority
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

redux: Discard messages/narrows on REALM_INIT, not DEAD_QUEUE. chrisbobbe/zulip-mobile
2 participants
@gnprice @chrisbobbe