Added some comments addressing the concerns from the Linear task https://linear.app/comm/issue/ENG-11232/review-performance-of-farcaster-sync

I'm not sure how we handle startReached for Farcaster threads. I think if we query for messages, but we receive 0, we should set startReached: true.

This is the same as for the thick threads. If we query the DB for messages and receive less than queried, we set startReached = true.

I'm not entirely sure how the order of operations will work for the effect if a fetchMessages call results in startReached: true. Ideally, this should prevent a second call to fetchMessages, but it's possible that setCanFetchOlderMessages(true) resolves quicker than the Redux update and triggers a second fetchMessages before startReached: true is considered.

Yeah, it is a valid concern, but it only applies when there is more than one batch. In that case, it is unlikely that the first batch doesn't have any message that can be shown in the preview. Also, we're fetching up to defaultNumberPerThread so it also reduces the probability of the issue happening.

In D15310#421640, @ashoat wrote:

Accepting to unblock, but I have some concerns below.

I'm not sure how we handle startReached for Farcaster threads. I think if we query for messages, but we receive 0, we should set startReached: true.

This is the same as for the thick threads. If we query the DB for messages and receive less than queried, we set startReached = true.

I'm confused by this response. Thick threads don't have a server you can query for more messages. For Farcaster threads, it's very possible to query the DB and receive less than queried messages... but in that case, it's still possible that there are more messages on the Farcaster backend. In that case, we should not be setting startReached: true. Is this logic broken for Farcaster threads?

After we connect the Farcaster DCs account, we're fetching all the threads and messages from the Farcaster servers. 20 most recent messages are added to Redux, the rest goes to the DB. We're doing that so that the messages are included in the backup. Then, all the new messages are received by a Tunnelbroker websocket and forwarded to clients. This all means that we never have to call the Farcaster servers when scrolling to older messages.

Yeah, it is a valid concern, but it only applies when there is more than one batch.

What do you mean by "batch"? Are you represented to unique query payloads passed to fetchMessages? I think the issue I described applies with just a single query payload to fetchMessages... basically, setCanFetchOlderMessages flips to true and triggers a re-query of the same exact data before the Redux update propagates.

What I meant is that this scenario can happen only if the most recent messages don't contain a message that can be shown in the preview. If a message that can be shown in the preview is found, then numOlderMessagesToFetch = 0 which means we will early exit from the effect because of if (!canFetchOlderMessages || numOlderMessagesToFetch === 0) { condition. So for your scenario to happen, there should be more messages in the DB than in the Redux - this is what I meant by the next batch.

Then, I think, your scenario could happen - if the result of fetchMessages contains a message we're looking for, we can accidentally ask for the next batch. But even then, I don't think it is a big issue because most of the messages can be shown in the preview, so the probability of not finding any such a message is low.

Have you tested this case?

No, I haven't. The overall performance of the solution for Farcaster threads is an order of magnitude better after this diff. Testing this is a bit inconvenient because of the need to create messages that can't be shown in the preview. Even if we realize we can improve something here, I think other things have much higher priorities.

Makes sense, thanks for explaining!