New messages simply disappear when starting the app without internet

[agrawal-d]

A spinner is shown when you were connected when you launched the app, but while staying on the app, became disconnected, and posted a new message

However, when starting the app already disconnected from the internet, any new message posted simply disappears, and no spinner/error is shown. Happens both in PMs and in Streams.

This message appears back upon re-connecting to the internet.

[chrisbobbe]

Thanks for the report, @agrawal-d! Possibly related is issue #3281. A PR was made to fix that issue, but it was reverted when some tests failed. Greg suspects the code change itself is OK, but the tests need to be changed.

[agrawal-d]

I tested #3281, it does not fix this issue.

[gnprice]

Interesting! I don't think we've otherwise known about this issue. Marking as a priority, because messages disappearing (even temporarily so that it looks like they have completely) is a bad symptom.

The one issue I see with similar symptoms is #3117. But those reports don't mention starting up without internet.

I think the next step for debugging this would be to identify what step is going wrong in our normal sequence for sending a message. This goes through addToOutbox in outboxActions.js; after the messageSendStart(..) action is dispatched, the message ought to be in Redux under state.outbox, and that should cause it to appear in the message list. So questions include:

• Is that happening? (If not, why not?)
• If yes, is the outbox message promptly disappearing again from state.outbox? (If so, why?)
• If not, why is the outbox message not appearing in the message list?

A good tool for answering some of these initial questions is to use RN's remote JS debugging and look at the console, at the information we have redux-logger log there. For questions that doesn't answer, I'd add some console.log calls.

[chrisbobbe]

I'm curious whether we might still be affected by the upstream issues with fetch that surfaced in zulip/react-native#2 (June 2018). That's a PR to our fork of react-native, which we'd actually abandoned in 4b95b12 (March 2017), in favor of using the published react-native. So, apparently the issues addressed in that PR were serious enough that we considered going back to our react-native fork after almost a year without it.

The PR is marked as fixing #2287, #2310, and #2315, and all three are still open and without activity since 2018.

@roberthoenig and @gnprice did a ton of really valuable debugging work that I haven't fully absorbed, but I'll look deeper if you'd like me to.

Robert's facebook/react-native issue, facebook/react-native#19709, is still open, and people are commenting to un-stale it.

Robert's square/okhttp issue, square/okhttp#4079, is also still open, and Robert has commented that the issue may be in okhttp, not react-native. An okhttp maintainer first commented that it might be an Android bug, but a subsequent comment from the same maintainer seemed to suggest a solution exists within okhttp.

[gnprice]

Hmm! Thanks for looking into that -- I hadn't thought about that upstream issue (and Robert's fix for it!) in a while. That might indeed be a useful thing for us to pick back up, and get the fix shipped.

Looking back at this particular issue, I don't actually see how that issue would cause this one, though. The effect of that upstream issue, when it bites, is I think that fetch takes a long time to complete. But no matter how delayed a fetch request might be, that shouldn't cause a sent message to disappear like this -- we should be keeping it around as an Outbox value until we can replace that with a Message value, however long that might be.

So I'd still be quite interested in learning answers to the debugging questions I suggested above.

[agrawal-d]

I investigated this issue today, and have answers to the first two questions:

I think the next step for debugging this would be to identify what step is going wrong in our normal sequence for sending a message. This goes through addToOutbox in outboxActions.js; after the messageSendStart(..) action is dispatched, the message ought to be in Redux under state.outbox, and that should cause it to appear in the message list. So questions include:

Is that happening? (If not, why not?)

Yes, the message appears in the outbox.

If yes, is the outbox message promptly disappearing again from state.outbox? (If so, why?)

No, it does not disappear from the outbox.

If not, why is the outbox message not appearing in the message list?

I'll need to investigate further to answer this.

[agrawal-d]

If not, why is the outbox message not appearing in the message list?

MessageList uses getShownMessagesForNarrow to load messages, which internally calls outboxMessagesForNarrow to load outbox messages, but it's returning a NULL_ARRAY in this case, so the message does not load:

narrowSelectors.js:34

export const outboxMessagesForNarrow: Selector<Outbox[], Narrow> = createSelector(
  (state, narrow) => narrow,
  getCaughtUpForNarrow,
  state => getOutbox(state),
  (narrow, caughtUp, outboxMessages) => {
    if (!caughtUp.newer) {
      return NULL_ARRAY;
    }
    const filtered = outboxMessages.filter(item => narrowContains(narrow, item.narrow));
    return isEqual(filtered, outboxMessages) ? outboxMessages : filtered;
  },
);

[agrawal-d]

Yep, temporarily removing

    if (!caughtUp.newer) {
      return NULL_ARRAY;
    }

fixes this issue.

[agrawal-d]

I don't get the logic behind the if condition I mentioned above - what I understand from it is: 'If we haven't fetched the newest messages, don't show unsent messages'.

That does not make much sense to me. In fact think there is no need for this condition at all, because whatever may be our perceived knowledge about the fetched messages, the app can still have unsent messages, and they should always be shown.

So, if I were to make a PR to fix this issue, I would remove this condition. Would that be correct?