Details
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| keyserver/src/fetchers/thread-fetchers.js | ||
|---|---|---|
| 317–323 ↗ | (On Diff #27396) | It doesn't appear that you're using the threads table for anything. In my original query I was querying for the name column, but since you only need id, it appears that you should be able to do this query just on memberships, without a LEFT JOIN. Let me know if I'm missing something |
| lib/reducers/message-reducer.js | ||
|---|---|---|
| 878–907 ↗ | (On Diff #27396) | I think we can merge these |
- Fixed small issues
- Instead of passing joinedThreads to fetchMessageInfo I instead add a condition to the sql query role > 0 (more context here)
- On the client threads are sorted according to lastUpdatedTimeIncludingSidebars. This means that:
- we could fetch last message for a sidebar
- client still doesn't have messages for the parent thread, so it's not displayed
- later client scrolls down and fetches last message for the parent thread
- parent thread in displayed, but it jumps up because it's sidebar has an earlier messages To fix that I also fetch the last message for parent threads of any sidebars. This complicates the lastMessage logic (I now have to filter the additional parent threads) so now it's done on the server and passed to the client.
| keyserver/src/fetchers/thread-fetchers.js | ||
|---|---|---|
| 311 | Hmm... one concern here is that we're fetching the parents for a sidebar, but not including "sibling sidebars" that might be displayed in the UI. If the user scrolls down and we load another sibling sidebar, that sibling would be displayed above where the user scrolled to, so they will miss it. I wonder if we should do some work to make sure we include all of the sidebars that would normally be rendered in the UI today (there is an algorithm for determing which sidebars to show here) | |
| keyserver/src/fetchers/thread-fetchers.js | ||
|---|---|---|
| 311 | It should be fine (but kinda brittle) in this case. The cutoff for displaying the sidebars is 3 days, while we stop sending messages after 2 weeks. So sidebars that would be fetched with this endpoint won't be displayed anyway. | |
| keyserver/src/fetchers/thread-fetchers.js | ||
|---|---|---|
| 311 | That appears to only be true if the sidebars are marked as read. If the sidebars are unread, it looks like we show up to 5 of them indefinitely | |
| keyserver/src/fetchers/thread-fetchers.js | ||
|---|---|---|
| 311 | Yes, but unread status is kept in threadInfo which is always available. And there's no problem with displaying sidebars without messages in messageStore because they don't have a preview. | |
| keyserver/src/fetchers/thread-fetchers.js | ||
|---|---|---|
| 311 | Then why are we querying for sidebars at all? It seems like what we want is a list of top-level chats, ordered by the most recent message in that chat or any of its sidebars. Instead, you're querying for a list of all chats (including sidebars), ordered by the most recent message in that chat. If you could find a MySQL query that gives you the result you want directly, we'll see the following benefits:
I assume such a query would have to be more complicated, so it would be good if you can give me an example query I can run on my production keyserver, so we can audit the performance of the new query. What do you think? | |
| keyserver/src/responders/message-responders.js | ||
| 560 | If we don't need any messages for sidebars (since the previews are not needed), then why are we querying for them here? | |
We are changing the approach for this task, so I'm abandoning this diff. More info in the linear task.