Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add some improvements to thread handling #8407

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add some improvements to thread handling #8407

wants to merge 2 commits into from

Conversation

larkox
Copy link
Contributor

@larkox larkox commented Dec 11, 2024

Summary

The following improvements has been added:

  • Default last_reply_at to the post create_at, if the thread is 0.
  • To get the limits, consider the post create_at if the last_reply_at is 0
  • Update the limits only if the received limits are smaller than the ones we have
  • Add a real refresh (get the last 60) functionality to the pull to refresh on threads
  • Always fetch the unreads when fetching threads (this may add stress to the server)
  • Only show threads with replies

This should solve several issues, but mainly tackling two:

  • Thread with replies and last_reply_at = 0 showing as 54 years old threads (and messing with the ordering)
  • "In between unreads" (outside of the last fetched page) not showing in the unreads tab
  • last_reply_at = 0 causing the "anti-gap" logic to fail
  • Add a "quick fix" for latest threads (refresh)
  • Threads without a reply showing in the thread list

This does NOT solve the following issues:

  • Potential gaps caused by the thread api returning the threads based on membership update_at but not surfacing that information
  • Refetching the same information over and over because the thread api returns the threads based on membership update_at but not surfacing that information
  • Threads still appear in the list after unfollowing them while the app is closed

Some of the issues may need a database purge (logout and log back in) to get fixed.

Ticket Link

Fix https://mattermost.atlassian.net/browse/MM-61759
Fix https://mattermost.atlassian.net/browse/MM-60745

Release Note

Improve reliability of threads

@larkox larkox added 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester labels Dec 11, 2024
@larkox larkox requested review from marianunez and enahum December 11, 2024 14:05
@enahum
Copy link
Contributor

enahum commented Dec 11, 2024

Threads still appear in the list after unfollowing them while the app is closed.

This is kind of bad. Is that happening today?

Also, I believe there is some code doing something kind of the same in the notifications area.

Have we looked at how these thread gaps are different than fetching posts on a channel?

Will review soon. Just wanted to add this comment before I forget.

@larkox
Copy link
Contributor Author

larkox commented Dec 11, 2024

This is kind of bad. Is that happening today?

I would swear it does. I have not changed any logic around that.

Also, I believe there is some code doing something kind of the same in the notifications area.

I looked how we were adding the threads, and I was not much worried about the last_reply_at since I expected the threads to be fetch again when the app ran.
After looking it more carefully, I see that we are indeed updating the TeamThreadSync table on the native side, so that should be handled too. If this is the approach we want to take, I will try to update those.

Have we looked at how these thread gaps are different than fetching posts on a channel?

With fetching posts we have a more reliable update at (the post update at). Here we don't have such a thing right now (AFAIK). Also, since we are receiving "0 replies threads" (and therefore we have a last reply at as 0) that was messing with the bottom end of the gaps, something that would not happen with posts (the update_at is always above 0).

@marianunez marianunez requested review from devinbinnie and removed request for marianunez December 11, 2024 20:19
Copy link
Member

@devinbinnie devinbinnie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple non-blocking questions otherwise LGTM.

const {earliestThread, latestThread} = getThreadsListEdges(latestThreads.threads);
syncDataUpdate.latest = latestThread.last_reply_at;
syncDataUpdate.earliest = earliestThread.last_reply_at;
syncDataUpdate.latest = latestThread.last_reply_at || latestThread.post.create_at;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we not sync data on post edits? Or is that done per post?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thread information in the database only includes the post id, so updates in the post should not affect this piece of data (it affects the post itself, which is done somewhere else).

@@ -291,7 +291,7 @@ export const syncThreadsIfNeeded = async (serverUrl: string, isCRTEnabled: boole

if (teams?.length) {
for (const team of teams) {
promises.push(syncTeamThreads(serverUrl, team.id, true, true));
promises.push(syncTeamThreads(serverUrl, team.id, {excludeDirect: true, fetchOnly: true}));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious - where do we sync the DM/GM threads?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other calls to syncTeamThreads, where we don't pass the excludeDirect option. Mainly on startup / reconnect, if I am not mistaken.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exclude direct is false also for the initial team, this will fetch the threads on DM's and GM's that are returned for every team if you don't set the flag as true

Copy link
Contributor

@enahum enahum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, but I still think we should look into the notifications area

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants