When message fetch fails, show error instead of loading animation #4205

chrisbobbe · 2020-07-24T02:07:21Z

There are some subtle things going on here, so I'll want to do some further testing and explaining (in commit messages and JSDocs), so I've marked this as a draft. But having code to look at will likely help along the conversation happening in chat.

So glad to be this much closer to rescuing people from the dreaded infinite loading indicator! 🙂

chrisbobbe · 2020-07-24T02:09:31Z

src/message/__tests__/messageActions-test.js

-      expect(action2.type).toBe('Navigation/PUSH');
-    });
-
-    test('when no messages in new narrow but caught up, only switch to narrow, do not fetch', () => {


Probably not great to remove these tests without something to replace them—it seems like these are mostly testing work that's the job of isFetchNeededAtAnchor? So, maybe I should write some tests for isFetchNeededAtAnchor.

chrisbobbe · 2020-07-24T02:15:20Z

src/chat/FetchError.js

@@ -0,0 +1,35 @@
+/* @flow strict-local */
+
+import React, { PureComponent } from 'react';


This file was copied from NoMessages; if anything looks weird, that's a likely reason.

chrisbobbe · 2020-07-24T02:17:07Z

Mm, from the issue:

Preferably also with a button (low-emphasis, like a text button) to retry.

I haven't done this yet, in this revision.

edit: I think this branch is complicated enough; let's maybe do this in a followup.

chrisbobbe · 2020-07-29T02:02:12Z

OK, and I just cleaned this up some more, so it's ready for a first review (I'm unmarking it as a draft). Still not sure how to proceed with fetchOlder and fetchNewer; brainstorming is ongoing here. I'm not sure whether it should block this PR or not. But I think users would be grateful for even an improvement that didn't yet handle fetchOlder and fetchNewer.

chrisbobbe · 2020-07-29T02:32:51Z

(Just addressed some things flagged in CI and pushed again.)

gnprice

Thanks @chrisbobbe !

I've read through all the prep commits (all commits but the last), and they look good with just the small comments below. Now studying the main commit at the end.

src/message/fetchActions.js

gnprice · 2020-08-05T00:38:53Z

src/message/fetchActions.js

+    // Just `return;` would be fine, but ESLint doesn't want us to do
+    // that while elsewhere returning something else.
+    return undefined;


I think this doesn't even need a comment. Just return; would result in the same behavior, but return undefined; is a way of being explicit that this codepath is choosing to return undefined, in a function that otherwise can return a more interesting value.

(Perhaps a shorter way to say this is: I agree with that ESLint rule 😉 )

src/message/__tests__/fetchActions-test.js

gnprice

[1] N.B.: If narrow X is considered the "topmost" or "last" narrow,
    and multiple `ChatScreen`s are showing narrow X, multiple
    fetches will be done (one per `ChatScreen` showing X). Where the
    number of `ChatScreen`s showing X is large, this is a
    performance regression.

Hmm interesting.

I think the performance regression shouldn't be an issue in practice, because even if you do manage to have a number of ChatScreens mounted at once (from having them all on the nav stack), they probably don't include a whole bunch of copies of the same narrow. But it certainly feels like a sign that something's still not quite modeled correctly after these changes.

I think specifically the issue is that the condition we'd like to have governing this isn't "is this ChatScreen showing the same narrow that's topmost or last", but rather "is this the topmost or last ChatScreen".

Some other comments below, and I'll also reply on other aspects in that chat thread.

src/chat/ChatScreen.js

gnprice · 2020-08-05T02:05:42Z

src/message/fetchActions.js

  dispatch(realmInit(initData, new ZulipVersion(serverSettings.zulip_version)));
-  dispatch(fetchTopMostNarrow());
  dispatch(initialFetchComplete());
  dispatch(startEventPolling(initData.queue_id, initData.last_event_id));

  dispatch(fetchPrivateMessages());

-  const session = getSession(getState());
-  if (session.lastNarrow) {
-    dispatch(fetchMessagesInNarrow(session.lastNarrow));
-  }
-


Huh, I hadn't quite spotted before that we do the "topmost narrow" fetch before INITIAL_FETCH_COMPLETE and the start of polling, and the "last narrow" fetch after those. (And the PM fetch after them as well.)

Seems likely there's a subtle bug in there, one way or the other.

gnprice · 2020-08-05T02:08:52Z

src/message/fetchActions.js

+ * up, or regaining a network connection. Mostly, we don't fetch
+ * messages here; that's dispatched from the UI component responsible
+ * for showing the messages and error states about the fetch. The


Probably best to mention one or both examples -- that way the reader who's interested in understanding how we maintain the messages data can go look there to follow along.

Seeing a similarity with `fetchSearchMessages` in src/search/SearchMessagesScreen, do something like 91e38a8 and allow spreading `fetchArgs` to avoid repetition. In upcoming commits, we'll nudge these similarities enough to show that it's basically code duplication, and fix that by calling `fetchMessages` from `fetchSearchMessages`.

We're about to have `fetchSearchMessages` call `fetchMessages` to avoid code duplication. To prepare for that to be an NFC change, basically drop in `fetchMessages`'s implementation here. There are 1.5 functional changes to comment on: - `fetchSearchMessages` didn't call MESSAGE_FETCH_START. This sounds wrong, so, fix it. It sounds wrong because MESSAGE_FETCH_START is meant to tell Redux that we're fetching messages. If we don't want the store (or parts of it) to update, that's a decision to be made in the reducers -- but we shouldn't skip making the announcement that we're fetching messages, especially since we already do update Redux when we get the response. Incidentally, there are a few places in the Redux store that we'd rather not update on a MESSAGE_FETCH_START for search narrows. Do that handling in the reducers, where it belongs, as we did for MESSAGE_FETCH_COMPLETE for search narrows. - We pass `use_first_unread_anchor: false`. It's always `false` because we use LAST_MESSAGE_ANCHOR as the anchor. But it defaults to `false` anyway, so this should be fine. [1] [1] We should help along the deprecation of this; that's zulip#4203.

Also assign some one-off messages to variables, for reuse. We'll be adding tests that use these soon.

Soon, we'll add another test that's interested in a successful fetch. After that, we'll add another section for when the fetch fails.

In an upcoming commit, it'll be handy to consume the messages as the (`Promise`d) return value of `dispatch` when you pass this function.

Much of `fetchSearchMessages`'s implementation is identical to that of `fetchMessages` in src/message/fetchActions.js (see the previous commits, where we basically dropped in the `fetchMessages` implementation with minimal edits). So, delete the comment that says it's not reusable, and reuse it. See discussion [1]. [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.23M4156.20Message.20List.20placeholders/near/928771

We'll use this in an upcoming commit, for zulip#4186.

It's normal and boring for `fetchMessages` to be dispatched when there's a logged-in account. So, represent that in the initial state. So, continue the work of 38aa238, and replace the cases we found in that commit with the slightly preferable `eg.selfAccount`; it's not the account of just any user, but the user who is logged in and active (the self). In the next commit, when `fetchMessages` starts dispatching the recently added MESSAGE_FETCH_ERROR on errors, we can add tests that do things like intentionally omit a logged-in account and make sure Redux is notified.

This should really be be done in all our async "thunk" action creators that themselves dispatch async "thunk" actions. That way, any errors (including errors in reducer code that run with plain old object actions as inputs) will propagate all the way up. See discussion [1]. [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.23M4156.20Message.20List.20placeholders/near/937480

Two tests for `doNarrow` implicitly test the behavior of `isFetchNeededAtAnchor`: - 'when no messages in new narrow but caught up, only switch to narrow, do not fetch' - 'when messages in new narrow are too few and not caught up, fetch more messages' It might have been better if `isFetchNeededAtAnchor` had had these focused tests to begin with. But we're about to have `doNarrow` never do a fetch, so it'll be quite unreasonable for them to still cover `isFetchNeededAtAnchor`. So, remove the tests, and add focused tests for `isFetchNeededAtAnchor`. Another thing that both removed tested was that a navigation action is dispatched with `doNarrow`. But this is already covered in the first `doNarrow` test.

For more straightforward error handling, move the dispatch of fetching messages to the UI component that we want to make responsible for handling errors in the fetch (and that is already responsible for showing the messages). Now, we can wrap the fetch in a `try` in the UI component itself, and easily set the local UI state in the `catch`. When we're in the error state, we'll see an error view instead of the infinite loading view, thus fixing zulip#4186. We have to carefully remove the fetches in `doInitialFetch` and `doNarrow` that this strategy replaces, and edit the tests accordingly. So, do. We fetch in `componentDidMount`, but that's not quite enough: we might need to reregister an event queue (see DEAD_QUEUE) while the component is just sitting there, having been mounted for some time. In particular, this needs to happen on REALM_INIT. This was handled before by `doInitialFetch` being called upon DEAD_QUEUE, and the fetch being done after dispatching REALM_INIT. So, now, we listen for REALM_INIT as a state change in `state.session.eventQueueId` in `componentDidUpdate`. We also preserve that the fetch on REALM_INIT only happens for the "topmost" or "last" narrows, using the same values that were used in `doInitialFetch`. We do a few things better: - Before, a separate fetch was done for the "topmost" *and* the "last" narrow, even if they were the same narrow. I haven't fully understood the differences between the two markers, but I suspect there's a lot of overlap. Now, a particular `ChatScreen` doesn't fetch twice, even if it's both "topmost" and "last" [1]. - Before, for non-"topmost" and non-"last" narrows, we didn't bother to fetch them at all, on reregistering an event queue -- even when they later became "topmost" or "last". So, set up a little bit of local UI state to say that we want them to fetch as soon as they next gain focus. Finally, adjust tests and comments according to the changes. See also discussion [2], where Greg carefully explains the subtle ways actions have to be ordered, and we discuss some race conditions. [1] N.B.: If narrow X is considered the "topmost" or "last" narrow, and multiple `ChatScreen`s are showing narrow X, multiple fetches will be done (one per `ChatScreen` showing X). Where the number of `ChatScreen`s showing X is large, this is a performance regression. This isn't likely to be an issue in practice, but Greg points out that it's a sign something's not quite modeled correctly, and we'll probably want to fix it: """ I think specifically the issue is that the condition we'd like to have governing this isn't "is this ChatScreen showing the same narrow that's topmost or last", but rather "is this the topmost or last ChatScreen". """ (from zulip#4205 (review)) [2] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.23M4156.20Message.20List.20placeholders/near/948514 Fixes: zulip#4186

gnprice · 2020-08-10T22:56:44Z

Looks good, merged, thanks!

As noted in discussion [1], we haven't been using quite the right information to decide whether it's time for a ChatScreen to fetch new messages. As Greg says, there, """ [T]he condition we'd like to have governing this isn't "is this ChatScreen showing the same narrow that's topmost or last", but rather "is this the topmost or last ChatScreen". """ To grab that bit, we first considered [2] something like adding a unique ID to each ChatScreen, which seems hacky. But while I was doing the recent React Navigation v3 and v4 upgrades, I came across this `withNavigationFocus` HOC, which provides an `isFocused` prop [3], which looks more or less exactly like what we'd want. "Is this ChatScreen focused" isn't *exactly* the same question as "is this the topmost or last ChatScreen", but the differences seem trivial, and I think this commit's approach is simpler and clearer. There's also an `isFocused` *method* directly on the `navigation` prop [4]; we get the `navigation` prop for free [5] since we've stuck `ChatScreen` directly on one of our navigators. The problem with just using that method is that `ChatScreen` doesn't naturally update (re-`render`) when it goes in and out of focus, so our code in `componentDidUpdate` that would ask for the `isFocused` value would never get run. So, use this HOC, which does set up `ChatScreen` to listen for a change of focus. [1] zulip#4205 (review) [2] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.23M4156.20Message.20List.20placeholders/near/978256 [3] https://reactnavigation.org/docs/4.x/function-after-focusing-screen#triggering-an-action-with-the-withnavigationfocus-higher-order-component [4] https://reactnavigation.org/docs/4.x/navigation-prop/#isfocused---query-the-focused-state-of-the-screen [5] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.23M4156.20Message.20List.20placeholders/near/953303

As noted in discussion [1], we haven't been using quite the right information to decide whether it's time for a ChatScreen to fetch new messages. As Greg says, there, """ [T]he condition we'd like to have governing this isn't "is this ChatScreen showing the same narrow that's topmost or last", but rather "is this the topmost or last ChatScreen". """ To grab that bit, we first considered [2] something like adding a unique ID to each ChatScreen, which seems hacky. But while I was doing the recent React Navigation v3 and v4 upgrades, I came across this `withNavigationFocus` HOC, which provides an `isFocused` prop [3], which looks more or less exactly like what we'd want. "Is this ChatScreen focused" isn't *exactly* the same question as "is this the topmost or last ChatScreen", but the differences seem trivial, and I think this commit's approach is simpler and clearer. See also Greg's thoughts on the difference [4]. There's also an `isFocused` *method* directly on the `navigation` prop [5]; we get the `navigation` prop for free [6] since we've stuck `ChatScreen` directly on one of our navigators. The problem with just using that method is that `ChatScreen` doesn't naturally update (re-`render`) when it goes in and out of focus, so our code in `componentDidUpdate` that would ask for the `isFocused` value would never get run. So, use this HOC, which does set up `ChatScreen` to listen for a change of focus. [1] zulip#4205 (review) [2] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.23M4156.20Message.20List.20placeholders/near/978256 [3] https://reactnavigation.org/docs/4.x/function-after-focusing-screen#triggering-an-action-with-the-withnavigationfocus-higher-order-component [4] zulip#4269 (review) [5] https://reactnavigation.org/docs/4.x/navigation-prop/#isfocused---query-the-focused-state-of-the-screen [6] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.23M4156.20Message.20List.20placeholders/near/953303

A `TimeoutError` will be handled the same way other errors in `fetchMessages` are handled; if it's a timeout in the fetch `ChatScreen` does on mount, `ChatScreen` will show the `FetchError` component we set up in zulip#4205. There's also been a passing mention on CZO of doing a timeout like this [1]: > After a long time, probably like a minute, we'll want that [...] > fetch to time out and fail in any case. [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.23M4156.20Message.20List.20placeholders/near/950853

chrisbobbe requested a review from gnprice July 24, 2020 02:07

chrisbobbe commented Jul 24, 2020

View reviewed changes

chrisbobbe force-pushed the fetch-error-ui branch from f12682a to 9f2848f Compare July 24, 2020 18:21

chrisbobbe mentioned this pull request Jul 28, 2020

Prep commits for new avatar_url handling (#4157) #4171

Merged

chrisbobbe force-pushed the fetch-error-ui branch 2 times, most recently from 126d139 to cda1cf9 Compare July 29, 2020 01:56

chrisbobbe marked this pull request as ready for review July 29, 2020 02:02

chrisbobbe force-pushed the fetch-error-ui branch from cda1cf9 to 2461067 Compare July 29, 2020 02:30

gnprice reviewed Aug 5, 2020

View reviewed changes

chrisbobbe added 14 commits August 7, 2020 16:54

example data: Add MESSAGE_FETCH_START to eg.action.

1e110d4

fetchActions tests [nfc]: Bring out initial state definition for reuse.

6b60f80

Also assign some one-off messages to variables, for reuse. We'll be adding tests that use these soon.

fetchActions [nfc]: File a test under new "success" describe block.

fb13098

Soon, we'll add another test that's interested in a successful fetch. After that, we'll add another section for when the fetch fails.

fetchActions: Have fetchMessages return the messages.

e5268bb

In an upcoming commit, it'll be handy to consume the messages as the (`Promise`d) return value of `dispatch` when you pass this function.

FetchError: Add, for showing an error in fetching messages.

353cd3a

fetchActions: Add MESSAGE_FETCH_ERROR action.

783a187

We'll use this in an upcoming commit, for zulip#4186.

fetchActions: Make fetchMessages alert Redux on failure.

3aabb1e

chrisbobbe force-pushed the fetch-error-ui branch from 2461067 to 4fde3cd Compare August 10, 2020 18:09

gnprice merged commit 4fde3cd into zulip:master Aug 10, 2020

gnprice added a-data-sync Zulip's event system, event queues, staleness/liveness a-message list P1 high-priority labels Aug 11, 2020

chrisbobbe mentioned this pull request Sep 24, 2020

ChatScreen: Get isFocused from React Navigation. #4269

Merged

chrisbobbe deleted the fetch-error-ui branch November 5, 2021 00:04

chrisbobbe mentioned this pull request Oct 4, 2023

api: Stop assuming fetched messages' reactions have deprecated user field #5773

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

When message fetch fails, show error instead of loading animation #4205

When message fetch fails, show error instead of loading animation #4205

chrisbobbe commented Jul 24, 2020 •

edited

Loading

chrisbobbe Jul 24, 2020

chrisbobbe Jul 29, 2020

chrisbobbe Jul 24, 2020

chrisbobbe commented Jul 24, 2020 •

edited

Loading

chrisbobbe commented Jul 29, 2020

chrisbobbe commented Jul 29, 2020

gnprice left a comment

gnprice Aug 5, 2020

gnprice left a comment

gnprice Aug 5, 2020

gnprice Aug 5, 2020

gnprice commented Aug 10, 2020

		@@ -0,0 +1,35 @@
		/* @flow strict-local */

		import React, { PureComponent } from 'react';

When message fetch fails, show error instead of loading animation #4205

When message fetch fails, show error instead of loading animation #4205

Conversation

chrisbobbe commented Jul 24, 2020 • edited Loading

chrisbobbe Jul 24, 2020

Choose a reason for hiding this comment

chrisbobbe Jul 29, 2020

Choose a reason for hiding this comment

chrisbobbe Jul 24, 2020

Choose a reason for hiding this comment

chrisbobbe commented Jul 24, 2020 • edited Loading

chrisbobbe commented Jul 29, 2020

chrisbobbe commented Jul 29, 2020

gnprice left a comment

Choose a reason for hiding this comment

gnprice Aug 5, 2020

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

gnprice Aug 5, 2020

Choose a reason for hiding this comment

gnprice Aug 5, 2020

Choose a reason for hiding this comment

gnprice commented Aug 10, 2020

chrisbobbe commented Jul 24, 2020 •

edited

Loading

chrisbobbe commented Jul 24, 2020 •

edited

Loading