Page MenuHomePhabricator

[lib] Fetch thick thread messages
ClosedPublic

Authored by tomek on Aug 23 2024, 10:52 AM.
Tags
None
Referenced Files
F3193256: D13162.id.diff
Fri, Nov 8, 10:37 PM
F3185837: D13162.diff
Fri, Nov 8, 1:44 PM
F3184890: D13162.id43624.diff
Fri, Nov 8, 12:22 PM
Unknown Object (File)
Mon, Nov 4, 6:49 PM
Unknown Object (File)
Sun, Nov 3, 2:34 AM
Unknown Object (File)
Sun, Nov 3, 2:33 AM
Unknown Object (File)
Wed, Oct 23, 7:23 PM
Unknown Object (File)
Wed, Oct 23, 6:21 PM
Subscribers

Details

Summary

Use the same mechanism as we use for the thin threads.

The change in message reducer is necessary because persisting the information about startReached status to SQLIte causes a bug where after refreshing the app we don't try to fetch messages ever again. An alternative solution would be to filter the status out on the DB layer, but I think this one is a lot cleaner.

https://linear.app/comm/issue/ENG-8706/modify-message-lists-to-call-the-new-function

Depends on D13147

Test Plan

On web:
Created a thick thread with 100 messages. Closed and reopened the app, scrolled up, and made sure all 100 messages are displayed.
There are two issues though:

  1. We don't stop after fetching the first batch. Instead, we're fetching all the batches one by one immediately. - created an issue to track https://linear.app/comm/issue/ENG-9097/fetching-new-messages-in-message-list-is-glitchy-when-it-happens
  2. The robotext about creating a thread isn't fetched when there are more than defaultNumberPerThread messages. - fixed. The issue was that we were always throwing away the last message, even in the last batch.

On native:
Created a thick thread with 100 messages. Closed and reopened the app, scrolled up, and made sure all 100 messages are displayed. This time issue 1. from the web isn't present because fetching (or updating the UI) is a lot slower.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

lib/actions/message-actions.js
590 ↗(On Diff #43624)

This is probably invalid (doesn't cause any bugs, but might be inefficient). I'll review if we should use a different value.

We don't stop after fetching the first batch. Instead, we're fetching all the batches one by one immediately.

This is caused by the fact that fetching from the DB is too fast. We probably have an existing bug, that wasn't caught, because fetching from keyservers is a lot slower.

Created https://linear.app/comm/issue/ENG-9097/fetching-new-messages-in-message-list-is-glitchy-when-it-happens to track, because it is an existing issue.

Fix fetching the last message

kamil added inline comments.
lib/actions/message-actions.js
32–35 ↗(On Diff #43664)

can be merged

lib/reducers/message-reducer.js
609–666 ↗(On Diff #43664)

could you add a code comment here explaining this?

This revision is now accepted and ready to land.Aug 27 2024, 3:27 AM

Simplify the logic and add a comment

This revision was automatically updated to reflect the committed changes.
lib/reducers/message-reducer.js
610–614

This is an interesting approach. It seems like if I were to look at SQLite for a thick thread containing more than defaultNumberPerThread messages, I would see startReached = false.

This is a bit confusing because the list of messages in SQLite is "complete" in the sense that all messages that the client can view are stored there. So I guess I would expect startReached = true for all thick threads in SQLite.

Did you consider any alternatives? For instance, I wonder if this hack could be applied at read time instead of at write time.

lib/reducers/message-reducer.js
610–614

I was considering the alternative where we modify the value at the read time, but it has a couple of issues:

  1. If all the thick threads had startReached = true then this flag would become completely useless
  2. It might be confusing to save one value to the DB and then read some other - in the current approach we read what we save
  3. The read query would become a bit complicated because reading threads would require counting messages

For me, both approaches make some sense but also could be confusing. The one from this diff is simpler to implement and less confusing to me.

lib/reducers/message-reducer.js
610–614

Thanks for explaining!