Page MenuHomePhabricator
Differential D14495

[lib] Fix highestTimestamp assignment in useGetLastUpdatedTimes
ClosedPublic
Actions

Authored by ashoat on Mon, Mar 24, 8:20 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Apr 1, 5:34 PM
Unknown Object (File)
Tue, Apr 1, 12:52 AM
Unknown Object (File)
Mon, Mar 31, 4:20 PM
Unknown Object (File)
Sun, Mar 30, 4:36 PM
Unknown Object (File)
Sun, Mar 30, 10:58 AM
Unknown Object (File)
Sat, Mar 29, 3:41 PM
Unknown Object (File)
Fri, Mar 28, 5:44 PM
Unknown Object (File)
Fri, Mar 28, 2:03 PM
View All 14 Files
Subscribers
None

Details

Reviewers
tomek
angelika
Commits
rCOMM481c30533aa3: [lib] Fix highestTimestamp assignment in useGetLastUpdatedTimes
Summary

highestTimestamp is used for setting lastUpdatedAtMostTime. The purpose of lastUpdatedAtMostTime is specifically for use in ChatThreadItemLoaderCache, where it specifies the order in which we test messages to get a final lastUpdatedTime.

We want to test messages in the order of lastUpdatedAtMostTime so that we can conclude when all of the non-resolves messages can be guaranteed to have a lower timestamp than the ones that have already resolved.

The current behavior here is broken because we're only setting highestTimestamp in cases where the time doesn't need to be resolved. We should be setting it to the absolute largest possible value that we could get after resolution, which is going to be the timestamp of the most recent message.

Depends on D14494

Test Plan

I tested this along with the rest of the stack by making sure that everything still worked on my physical device with a release build, and that performance was not degraded

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Revision Contents
Changeset List

PathSize
lib/
hooks/
21 lines

Diff 47543

View Options

lib/hooks/thread-time.js

Show All 32 Lines (threadInfo, messageStore) => {
if (!thread || !viewerID || thread.messageIDs.length === 0) { if (!thread || !viewerID || thread.messageIDs.length === 0) {
return { return {
lastUpdatedAtLeastTime: threadInfo.creationTime, lastUpdatedAtLeastTime: threadInfo.creationTime,
lastUpdatedAtMostTime: threadInfo.creationTime, lastUpdatedAtMostTime: threadInfo.creationTime,
lastUpdatedTime: () => Promise.resolve(threadInfo.creationTime), lastUpdatedTime: () => Promise.resolve(threadInfo.creationTime),
}; };
} }
const mostRecentMessage = messageStore.messages[thread.messageIDs[0]];
const lastUpdatedAtMostTime =
mostRecentMessage?.time ?? threadInfo.creationTime;
// We default to the most recent message time. // We default to the most recent message time.
// - This default will only be relevant in the case that // - This default will only be relevant in the case that
// getLastUpdatedTime is implemented for every message in the loop // getLastUpdatedTime is implemented for every message in the loop
// below, and doesn't return a number for any of them. Otherwise // below, and doesn't return a number for any of them. Otherwise
// lastUpdatedAtLeastTime will be set to the most recent message that // lastUpdatedAtLeastTime will be set to the most recent message that
// returns a number, or whose message spec doesn't implement // returns a number, or whose message spec doesn't implement
// getLastUpdatedTime. // getLastUpdatedTime.
// - We default to the most recent message time because the best // - We default to the most recent message time because the best
// alternative is the thread creation time, but this is likely very // alternative is the thread creation time, but this is likely very
// far in the past. // far in the past.
const mostRecentMessage = messageStore.messages[thread.messageIDs[0]]; let lastUpdatedAtLeastTime = lastUpdatedAtMostTime;
let lastUpdatedAtLeastTime =
mostRecentMessage?.time ?? threadInfo.creationTime;
const getLastUpdatedTimeParams = { const getLastUpdatedTimeParams = {
threadInfo, threadInfo,
viewerID, viewerID,
getMessageAuthor: (messageID: string) => getMessageAuthor: (messageID: string) =>
baseGetMessageAuthor(messageID, messageStore), baseGetMessageAuthor(messageID, messageStore),
}; };
let lastUpdatedTime: ?() => Promise<?number>; let lastUpdatedTime: ?() => Promise<?number>;
let highestTimestamp: ?number;
for (const messageID of thread.messageIDs) { for (const messageID of thread.messageIDs) {
const messageInfo = messageStore.messages[messageID]; const messageInfo = messageStore.messages[messageID];
if (!messageInfo) { if (!messageInfo) {
continue; continue;
} }
// We call getLastUpdatedTime on the message spec. It can return either // We call getLastUpdatedTime on the message spec. It can return either
// ?number or () => Promise<?number>. If the message spec doesn't // ?number or () => Promise<?number>. If the message spec doesn't
// implement getLastUpdatedTime, then we default to messageInfo.time. // implement getLastUpdatedTime, then we default to messageInfo.time.
const { getLastUpdatedTime } = messageSpecs[messageInfo.type]; const { getLastUpdatedTime } = messageSpecs[messageInfo.type];
const lastUpdatedTimeResult = getLastUpdatedTime const lastUpdatedTimeResult = getLastUpdatedTime
? getLastUpdatedTime(messageInfo, getLastUpdatedTimeParams) ? getLastUpdatedTime(messageInfo, getLastUpdatedTimeParams)
: messageInfo.time; : messageInfo.time;
// We only need to consider the first positive number result because
// thread.messageIDs is ordered chronologically.
if (
!highestTimestamp &&
lastUpdatedTimeResult &&
typeof lastUpdatedTimeResult === 'number'
) {
highestTimestamp = lastUpdatedTimeResult;
}
// We rely on the fact that thread.messageIDs is ordered chronologically // We rely on the fact that thread.messageIDs is ordered chronologically
// (newest first) to chain together lastUpdatedTime. An older message's // (newest first) to chain together lastUpdatedTime. An older message's
// lastUpdatedTime is only considered if all of the newer messages // lastUpdatedTime is only considered if all of the newer messages
// return falsey. // return falsey.
const prevLastUpdatedTime = lastUpdatedTime; const prevLastUpdatedTime = lastUpdatedTime;
lastUpdatedTime = async () => { lastUpdatedTime = async () => {
if (prevLastUpdatedTime) { if (prevLastUpdatedTime) {
const earlierChecks = await prevLastUpdatedTime(); const earlierChecks = await prevLastUpdatedTime();
Show All 27 Lines (threadInfo, messageStore) => {
const earlierChecks = await lastUpdatedTime(); const earlierChecks = await lastUpdatedTime();
if (earlierChecks) { if (earlierChecks) {
return earlierChecks; return earlierChecks;
} }
} }
return lastUpdatedAtLeastTime; return lastUpdatedAtLeastTime;
}; };
const lastUpdatedAtMostTime = highestTimestamp ?? threadInfo.creationTime;
return { return {
lastUpdatedAtLeastTime, lastUpdatedAtLeastTime,
lastUpdatedAtMostTime, lastUpdatedAtMostTime,
lastUpdatedTime: lastUpdatedWithFallback, lastUpdatedTime: lastUpdatedWithFallback,
}; };
}, },
[viewerID, baseGetMessageAuthor], [viewerID, baseGetMessageAuthor],
); );
} }
export { useGetLastUpdatedTimes }; export { useGetLastUpdatedTimes };