-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Changes from all commits
72c4132
1e4fcb3
0a09a65
b546925
d3b06d1
df650b1
c9f83d9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -300,7 +300,7 @@ export const syncThreadsIfNeeded = async ( | |
|
||
if (teams?.length) { | ||
for (const team of teams) { | ||
promises.push(syncTeamThreads(serverUrl, team.id, true, true, groupLabel)); | ||
promises.push(syncTeamThreads(serverUrl, team.id, {excludeDirect: true, fetchOnly: true, groupLabel})); | ||
} | ||
} | ||
|
||
|
@@ -325,9 +325,22 @@ export const syncThreadsIfNeeded = async ( | |
} | ||
}; | ||
|
||
type SyncThreadOptions = { | ||
excludeDirect?: boolean; | ||
fetchOnly?: boolean; | ||
refresh?: boolean; | ||
groupLabel?: string; | ||
} | ||
|
||
export const syncTeamThreads = async ( | ||
serverUrl: string, teamId: string, | ||
excludeDirect = false, fetchOnly = false, groupLabel?: string, | ||
serverUrl: string, | ||
teamId: string, | ||
{ | ||
excludeDirect = false, | ||
fetchOnly = false, | ||
refresh = false, | ||
groupLabel, | ||
}: SyncThreadOptions = {}, | ||
) => { | ||
try { | ||
const {database, operator} = DatabaseManager.getServerDatabaseAndOperator(serverUrl); | ||
|
@@ -368,39 +381,56 @@ export const syncTeamThreads = async ( | |
return {error: allUnreadThreads.error || latestThreads.error}; | ||
} | ||
|
||
const dedupe = new Set(latestThreads.threads?.map((t) => t.id)); | ||
|
||
if (latestThreads.threads?.length) { | ||
// We are fetching the threads for the first time. We get "latest" and "earliest" values. | ||
// At this point we may receive threads without replies, so we also check the post.create_at timestamp. | ||
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; | ||
syncDataUpdate.earliest = earliestThread.last_reply_at || earliestThread.post.create_at; | ||
|
||
threads.push(...latestThreads.threads); | ||
} | ||
if (allUnreadThreads.threads?.length) { | ||
const dedupe = new Set(latestThreads.threads?.map((t) => t.id)); | ||
const unread = allUnreadThreads.threads.filter((u) => !dedupe.has(u.id)); | ||
threads.push(...unread); | ||
} | ||
} else { | ||
const allNewThreads = await fetchThreads( | ||
serverUrl, | ||
teamId, | ||
{deleted: true, since: syncData.latest + 1, excludeDirect}, | ||
undefined, | ||
undefined, | ||
groupLabel, | ||
); | ||
const [allUnreadThreads, allNewThreads] = await Promise.all([ | ||
fetchThreads( | ||
serverUrl, | ||
teamId, | ||
{unread: true, excludeDirect}, | ||
Direction.Down, | ||
undefined, | ||
groupLabel, | ||
), | ||
fetchThreads( | ||
serverUrl, | ||
teamId, | ||
{deleted: true, since: refresh ? undefined : syncData.latest + 1, excludeDirect}, | ||
undefined, | ||
1, | ||
groupLabel, | ||
), | ||
]); | ||
|
||
if (allNewThreads.error) { | ||
return {error: allNewThreads.error}; | ||
} | ||
if (allNewThreads.threads?.length) { | ||
// As we are syncing, we get all new threads and we will update the "latest" value. | ||
const {latestThread} = getThreadsListEdges(allNewThreads.threads); | ||
syncDataUpdate.latest = latestThread.last_reply_at; | ||
const latestDate = latestThread.last_reply_at || latestThread.post.create_at; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How come we don't do latestThread.post.lastReplyAt like we did in transformer/thread.ts?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The piece of code you pasted says:
The code you are referencing is doesn't deal with what we used to have, so it only checks:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is no post last reply at, there is only thread last reply at. |
||
syncDataUpdate.latest = Math.max(syncData.latest, latestDate); | ||
|
||
threads.push(...allNewThreads.threads); | ||
} | ||
if (allUnreadThreads.threads?.length) { | ||
const dedupe = new Set(allNewThreads.threads?.map((t) => t.id)); | ||
const unread = allUnreadThreads.threads.filter((u) => !dedupe.has(u.id)); | ||
threads.push(...unread); | ||
} | ||
} | ||
|
||
const models: Model[] = []; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,7 +32,7 @@ const enhanced = withObservables(['tab', 'teamId'], ({database, tab, teamId}: Pr | |
threads: teamThreadsSyncObserver.pipe( | ||
switchMap((teamThreadsSync) => { | ||
const earliest = tab === 'all' ? teamThreadsSync?.[0]?.earliest : 0; | ||
return queryThreadsInTeam(database, teamId, getOnlyUnreads, false, true, true, earliest).observe(); | ||
return queryThreadsInTeam(database, teamId, getOnlyUnreads, true, true, true, earliest).observe(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mind explaining why changing hasReplies to true would improve thread handling? You'd get posts to appear in the thread list only when reply_count > 0?. I followed a single post with no reply and it's not showing in my thread list. Is adding this make a difference? Again curious how it improves. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The test case I know for this is:
You will see a thread without responses |
||
}), | ||
), | ||
}; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,7 +35,9 @@ export function processIsCRTEnabled(preferences: PreferenceModel[]|PreferenceTyp | |
export const getThreadsListEdges = (threads: Thread[]) => { | ||
// Sort a clone of 'threads' array by last_reply_at | ||
const sortedThreads = [...threads].sort((a, b) => { | ||
return a.last_reply_at - b.last_reply_at; | ||
const aDate = a.last_reply_at || a.post.create_at; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here... should we include a.post.lastReplyAt? |
||
const bDate = b.last_reply_at || b.post.create_at; | ||
return aDate - bDate; | ||
}); | ||
|
||
const earliestThread = sortedThreads[0]; | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).