Prevent fetching topics list if we already have them

[shaurya2612]

Fixes : #2750

The Problem - Topics list was fetched for the current stream each time the TextInput was focussed, irrespective of if we have fetched the data before or not. Take a look at #2750 for more...

Video of the problem -
https://user-images.githubusercontent.com/36705866/102719650-2e97b300-4315-11eb-90d4-c6a4a15fcb39.mov

Fix - Now we fetch data only once for a specific stream. The query is made each time we move to a different stream

After fixing-
https://user-images.githubusercontent.com/36705866/102719838-68b58480-4316-11eb-90f1-b5e7b3a45f78.mov

[chrisbobbe]

Thanks, @shaurya2612!

I think there might be a slightly simpler way to fix this, one that doesn't involve storing something in state. Would it work if we fetched unconditionally (for stream narrows anyway) at the start, probably in componentDidMount, and then fetched again in componentDidUpdate if the stream has changed?

[shaurya2612]

@chrisbobbe thanks for your perusal

Working on it now!

[shaurya2612]

@chrisbobbe I omitted the state variable and achieved the same functionality with componentDidMount and componentDidUpdate as you mentioned.

Thanks a lot !

[chrisbobbe]

Great, thanks, @shaurya2612! I've had a few more thoughts after looking a bit closer.

---

It occurred to me that, without frequently re-fetching the topics, the list of topics might get to be unhelpfully out-of-date. New messages that come into the stream might have topics we haven't seen before, and we'll want those to show up in the list.

Thankfully, I found that this is already handled. The list of topics mentioned in #2750 is rendered in the TopicAutocomplete component, which takes its data from the topics key in the Redux state; TopicAutocomplete subscribes to its data with a call to react-redux's connect. The effect is that TopicAutocomplete updates whenever the list of topics for the stream (i.e., the list of topics at state.topics[streamId] for the appropriate streamId) changes. (The data fetched by fetchTopicsForStream also gets stored in state.topics [1].)

So, we just need to be sure that state.topics[streamId] (for the appropriate streamId) is also updated when a new message, with a potentially new topic, is received. This is already the case: src/topics/topicsReducer.js, which is responsible for updating state.topics, does indeed handle EVENT_NEW_MESSAGE, and it does so by updating the list of topics for the new message's stream.

So, when a new message with a potentially new topic is received, the list of topics will indeed be kept up-to-date, and we don't need to do anything further about it. EDIT: Actually, it would probably be helpful to add a code comment where we call fetchTopicsForStream, saying that state.topics is updated on EVENT_NEW_MESSAGE actions, to memoize the results of this investigation. 🙂

---

One thing I also noticed in the above investigation is that the code that's fetching the topics, in ComposeBox, is quite far away from the code that's interested in the fetched topics, in TopicAutocomplete. Let's move it closer: I think the code that calls fetchTopicsForStream could helpfully be moved from ComposeBox to TopicAutocomplete. That'll also help keep the complexity of ComposeBox down; there's already quite a lot of code in that file.

---

Also, I think we don't actually need to re-fetch the topics when the stream changes; i.e., the componentDidUpdate from my suggestion above should probably just be removed. The reason I say so is that we don't expect the stream to actually change, and it would pretty much be a bug if it did. ComposeBox (and TopicAutocomplete, which it renders unconditionally) are descendants of ChatScreen, and a ChatScreen receives (and is meant to receive) a single narrow which will direct its behavior through its entire lifetime, from the time it's "born" with a call to navigateToChat containing the requested narrow in the navigation params. [2] Including the componentDidUpdate would only add confusion, I think; it would distort a reader's idea of what we normally expect to happen.

---

[1] I saw that the fetchTopicsForStream thunk action does the data fetch, then I command-clicked through fetchTopics to its definition and saw that it dispatches an INIT_TOPICS plain-object action with the topics it's just learned about; I then did a whole-project search for case INIT_TOPICS, which showed that src/topics/topicsReducer.js, the reducer for the topics key, is the only thing that handles INIT_TOPICS, and it does so by storing the newly received topics.

[2] There's a small wrinkle in getTopicsForNarrow, where it looks like we...won't show the list of topics if someone has changed the stream name—but that's something we're tracking and will be fixed with #4333.

[chrisbobbe]

Also, please be sure to run tools/test and fix any/all issues it tells you about (with the current revision, it identified a few issues with the lint and prettier tests; the former can be fixed manually, and the latter should be fixable automatically with tools/fmt which is mentioned in this doc).

Also, I think everything should be squashable into a single commit, which would be good to do in keeping with our commit discipline. We have a handy guide for revising your local branch ("rewriting history" for the PR branch, so you don't need to make a new PR) here. 🙂

[shaurya2612]

Thanks @chrisbobbe !
I've made the necessary changes and ran the tests as well. Please let me know what you think of these.

[chrisbobbe]

Thanks, @shaurya2612!

Merged, with a few tweaks to the commit message; please take a look 🙂 (and see our doc on our preferred commit-message style):

• I've added some more detail to the commit message.
• Formatted the summary line with a prefix; I chose ComposeBox, since that's the site where we're removing the troublesome too-frequent fetch.
• Kept the line-wrapping of the body to about 68 characters (I've got my editor set to do this semi-automatically).
• Added Fixes: #2750 at the end.

I also expanded on the code comment you added, so there's more context on the important fact you mention (that "state.topics is updated on EVENT_NEW_MESSAGE actions").

(

And I added some quick TODOs from the discovery in #4351 (comment):

[2] There's a small wrinkle in getTopicsForNarrow, where it looks like we...won't show the list of topics if someone has changed the stream name—but that's something we're tracking and will be fixed with #4333.

)