redux: Discard messages/narrows on REALM_INIT, not DEAD_QUEUE.

[chrisbobbe]

Fixes: #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.

[chrisbobbe]

This is WIP because, while my manual testing hasn't revealed any unintended side effects, this touches some central logic, so should be tested more thoroughly, and I have a few lingering questions about redux-persist (to be posted shortly).

[chrisbobbe]

Specifically, from reading the code, it seems like redux-persist is used to rehydrate the Redux store with data from AsyncStorage, right? Are state.messages and/or state.narrows persisted in this way, and if so, does that matter?

I'm not sure what the order is of the REHYDRATE action and our other actions, including the ones in doInitialFetch. 42adfb3 describes createActionBuffer as a way to postpone other actions so REHYDRATE always happens first, but I'm not sure that's exactly what it does. From the doc:

A middleware for redux that buffers all actions into a queue until a breaker condition is met, at which point the queue is released (i.e. actions are triggered).

Our breaker condition is the REHYDRATE action, but from that description, it doesn't seem like there's any reordering of the queue (e.g., for our case, to put REHYDRATE at the front), only that the queue doesn't start emptying until the breaker condition is met (for us, when REHYDRATE is dispatched -- potentially with other actions already ahead of it in the queue).

Does this seem right, and if so, might it matter (either in general or for this PR)?

[chrisbobbe]

(Just made some edits to the commit message.)

[gnprice]

Are state.messages and/or state.narrows persisted in this way, and if so, does that matter?

I'm not sure what the order is of the REHYDRATE action and our other actions, [...]

We discussed these questions yesterday in person, and I think I answered them then. I've also just sent #3907 which adds a bunch of jsdoc and other comments to store.js, which hopefully will help make these (and many other questions) easy for the next person to find the answers to in the future.

[chrisbobbe]

Ah, we were writing comments at the same time! 🙂 I'll post mine anyway, just to make sure I've understood correctly.

Does this seem right, and if so, might it matter (either in general or for this PR)?

This makes more sense, after discussing in person. The REHYDRATE action marks the completion of retrieving the state from ZulipAsyncStorage, for the whitelisted fields specified in storeKeys and cacheKeys. It's clear from reading the source code that all of the actions that would have preceded REHYDRATE are entered into a queue, which is released after REHYDRATE (not before).

In fact, I may have been a little too harsh on the docs yesterday — it turns out that the "breaker condition" can be anything, not just a Redux action. The fact that it happens to be an action for us made me think, ok, here's a library intended to manage a homogeneous queue of Redux actions (including REHYDRATE), but it just likes to break the definition of a queue, to suit some purpose, by reordering REHYDRATE to the front. In reality, the buffered actions are managed properly in a queue; REHYDRATE was just never considered part of the queue in the first place.

[chrisbobbe]

OK, I've read and merged PR #3900. I also read PR #3907.

[gnprice]

Thanks @chrisbobbe!

I just reread this PR, in light of #3900. (Which, for self-containedness of this thread, was this:

I sat down this afternoon to look at #3822 and #3802, and found I wasn't 100% sure under what conditions we show that "No Messages" message, and that it wasn't immediate to find out.

I dug into the code to answer that question, and this series of changes is the result. 🙂

) The main question in my mind was: after this PR, when you open the app with an expired event queue, what's the sequence of changes that would affect whether we show "No messages"? And should we expect that there will still be a stage at which we do show that?

I think the answer is: this change narrows that window by a lot, but doesn't quite close it; and there's an additional tweak we can do that will then close it.

Specifically, this is what now decides when that's shown (in Chat.js):

    const isFetching = fetching.older || fetching.newer;
    const showMessagePlaceholders = haveNoMessages && isFetching;
    const sayNoMessages = haveNoMessages && !isFetching;

where fetching is the per-narrow value from state.fetching, and haveNoMessages is msgs.length === 0 where msgs is the per-narrow array of messages we would show in the message list for this narrow.

The issue this PR is aimed at is basically that we're making sayNoMessages true when we shouldn't.

The PR works by delaying the point when haveNoMessages becomes true. In the sequence in doInitialFetch:

export const doInitialFetch = () => async (dispatch: Dispatch, getState: GetState) => {
  // ...

  dispatch(initialFetchStart());
  const auth = getAuth(getState());

  // ...
    initData = await tryUntilSuccessful(() =>
      api.registerForEvents(auth, {
        fetch_event_types: config.serverDataOnStartup,
        apply_markdown: true,
        include_subscribers: false,
        client_gravatar: true,
        client_capabilities: {
          notification_settings_null: true,
        },
      }),
    );

  dispatch(realmInit(initData));
  dispatch(fetchTopMostNarrow());
  dispatch(initialFetchComplete());

it moves to the realmInit step, whereas in master it's happening just before doInitialFetch is even called.

Just after the realmInit step, I'd expect this would still leave us saying "No messages" -- at least if Chat#render happens to get called at that moment. We're clearing out the narrows and messages state trees, so haveNoMessages will be true; but isFetching will still be false. It will be made true by fetchTopMostNarrow, which will start a fetch for the narrow currently shown on screen.

Hmm, though perhaps it's not actually possible for render to get called in between those! I think realmInit is all synchronous, and so is the parts of fetchTopMostNarrow up through dispatching the messageFetchStart event that causes isFetching to become true. In principle, all those dispatch calls give Redux an opportunity to run any code it likes, including a react-redux listener that in turn causes React to call render... but it's possible that Redux or react-redux makes a guarantee that won't happen. In which case this does already completely fix the issue.

Still, even if that turns out to be the case, the whole chain of reasoning feels a bit fragile -- dependent on fine details of the sequence of calls in doInitialFetch and of the exact places await appears in the definitions of its various helpers.

I think a good fix would be to augment the definition of isFetching in that Chat.js logic quoted above, so that it also considers state.session.loading, using getLoading. That's the flag which is set by initialFetchStart and cleared by initialFetchComplete; it basically covers "we're doing a fresh /register call, loading a fresh copy of the overall app data". I think it makes semantic sense that if we're in the middle of that and we don't have any messages, we should emphasize "we're loading messages" rather than "we have no messages" -- which is exactly the effect of showMessagePlaceholders vs. sayNoMessages in that Chat#render code.

[gnprice]

A separate (simpler 🙂 ) question I looked at this to answer: is this acting together on all the data that's closely related and we want to act together on?

So I went and looked at reduxTypes.js, which has some useful cross-references.

 * These `Message` objects lack the `flags` property; we store that
 * information separately, as `FlagsState`.
 *
 * See also `NarrowsState`, which is an index on this data that identifies
 * messages belonging to a given narrow.
 */
export type MessagesState = {|
// ...

 * See also:
 *  * `MessagesState`, which stores the message data indexed by ID;
 *  * `CaughtUpState` for information about where this data *is*
 *    complete for a given narrow;
 *  * `FetchingState` for information about which narrows we're actively
 *    fetching more messages from.
 */
export type NarrowsState = {

So I think at least FlagsState i.e. state.flags should be tracked the same way as MessagesState i.e. state.messages. ... Looks like there's already an unrelated discrepancy there, in the handling of LOGIN_SUCCESS. But in any case flagsReducer.js should get the same update in this diff as messagesReducer.js does.

And then also caughtUp should get cleared when narrows is.

As for fetching... I'm not convinced it should properly be cleared on either DEAD_QUEUE or REALM_INIT! Neither one actually causes us to stop an ongoing fetch. So probably delete that case line from fetchingReducer.

[chrisbobbe]

All good points, thanks for explaining all of that! I've made changes to address these things.

[gnprice]

And merged -- thanks!