From 99f025db188ac20791ef130fe8aea13be09fdac2 Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Wed, 28 Apr 2021 12:14:03 -1000 Subject: [PATCH 01/13] Use offset when fetching actions on reconnect add way to extract collection key use array in fetchOrCreateChatReport fix return types Add Timers namespace so there is some way to prevent delayed requests on sign out undo changes revert login form change undo sleep timer change again --- src/libs/CollectionUtils.js | 16 ++++- .../Navigation/AppNavigator/AuthScreens.js | 5 +- src/libs/Timers.js | 31 +++++++++ src/libs/actions/PersonalDetails.js | 2 +- src/libs/actions/Report.js | 67 +++++++++++++------ src/libs/actions/Session.js | 2 + src/libs/actions/SignInRedirect.js | 7 -- 7 files changed, 98 insertions(+), 32 deletions(-) create mode 100644 src/libs/Timers.js diff --git a/src/libs/CollectionUtils.js b/src/libs/CollectionUtils.js index b4501d1a5dee..e1f5faead1ba 100644 --- a/src/libs/CollectionUtils.js +++ b/src/libs/CollectionUtils.js @@ -8,11 +8,23 @@ import _ from 'underscore'; * @param {Object} object * @return {*} */ -export function lastItem(object = {}) { +function lastItem(object = {}) { const lastKey = _.last(_.keys(object)) || 0; return object[lastKey]; } -export default { +/** + * Used to grab the id for a particular collection item's key. + * e.g. reportActions_1 -> 1 + * + * @param {String} key + * @returns {String} + */ +function extractCollectionItemID(key) { + return key.split('_')[1]; +} + +export { lastItem, + extractCollectionItemID, }; diff --git a/src/libs/Navigation/AppNavigator/AuthScreens.js b/src/libs/Navigation/AppNavigator/AuthScreens.js index 0726b2cdc949..2f202da5ba83 100644 --- a/src/libs/Navigation/AppNavigator/AuthScreens.js +++ b/src/libs/Navigation/AppNavigator/AuthScreens.js @@ -48,6 +48,7 @@ import { SettingsModalStackNavigator, } from './ModalStackNavigators'; import SCREENS from '../../../SCREENS'; +import Timers from '../../Timers'; Onyx.connect({ key: ONYXKEYS.MY_PERSONAL_DETAILS, @@ -120,14 +121,14 @@ class AuthScreens extends React.Component { // Refresh the personal details, timezone and betas every 30 minutes // There is no pusher event that sends updated personal details data yet // See https://github.com/Expensify/ReactNativeChat/issues/468 - this.interval = setInterval(() => { + this.interval = Timers.register(setInterval(() => { if (this.props.network.isOffline) { return; } PersonalDetails.fetch(); User.getUserDetails(); User.getBetas(); - }, 1000 * 60 * 30); + }, 1000 * 60 * 30)); Timing.end(CONST.TIMING.HOMEPAGE_INITIAL_RENDER); diff --git a/src/libs/Timers.js b/src/libs/Timers.js new file mode 100644 index 000000000000..560ea154d169 --- /dev/null +++ b/src/libs/Timers.js @@ -0,0 +1,31 @@ +import _ from 'underscore'; + +const timers = []; + +/** + * Register a timer so it can be cleaned up later. + * + * @param {Number} timerID + * @return {Number} + */ +function register(timerID) { + timers.push(timerID); + return timerID; +} + +/** + * Clears all timers that we have registered. Use for long running tasks that may begin once logged out. + */ +function clearAll() { + _.each(timers, (timer) => { + // We don't know whether it's a setTimeout or a setInterval, but it doesn't really matter. If the id doesn't + // exist nothing bad happens. + clearTimeout(timer); + clearInterval(timer); + }); +} + +export default { + register, + clearAll, +}; diff --git a/src/libs/actions/PersonalDetails.js b/src/libs/actions/PersonalDetails.js index 1fb9512c86c2..481281fb0e8b 100644 --- a/src/libs/actions/PersonalDetails.js +++ b/src/libs/actions/PersonalDetails.js @@ -61,7 +61,7 @@ function getDisplayName(login, personalDetail) { // If we have a number like +15857527441@expensify.sms then let's remove @expensify.sms // so that the option looks cleaner in our UI. const userLogin = Str.removeSMSDomain(login); - const userDetails = personalDetail || personalDetails[login]; + const userDetails = personalDetail || lodashGet(personalDetails, login); if (!userDetails) { return userLogin; diff --git a/src/libs/actions/Report.js b/src/libs/actions/Report.js index 1058e9a05efe..744cc3ba9f36 100644 --- a/src/libs/actions/Report.js +++ b/src/libs/actions/Report.js @@ -18,6 +18,8 @@ import * as API from '../API'; import CONST from '../../CONST'; import Log from '../Log'; import {isReportMessageAttachment} from '../reportUtils'; +import * as CollectionUtils from '../CollectionUtils'; +import Timers from '../Timers'; let currentUserEmail; let currentUserAccountID; @@ -54,6 +56,24 @@ Onyx.connect({ }, }); +const mostRecentStoredSequenceNumbers = {}; +Onyx.connect({ + key: ONYXKEYS.COLLECTION.REPORT_ACTIONS, + callback: (val, key) => { + if (!key || !val) { + return; + } + + const reportID = CollectionUtils.extractCollectionItemID(key); + const mostRecentAction = CollectionUtils.lastItem(val); + if (!mostRecentAction || _.isUndefined(mostRecentAction.sequenceNumber)) { + return; + } + + mostRecentStoredSequenceNumbers[reportID] = mostRecentAction.sequenceNumber; + }, +}); + const typingWatchTimers = {}; // Keeps track of the max sequence number for each report @@ -268,7 +288,7 @@ function fetchIOUReportID(debtorEmail) { * chat report IDs * * @param {Array} chatList - * @return {Promise} only used internally when fetchAllReports() is called + * @returns {Promise} only used internally when fetchAllReports() is called */ function fetchChatReportsByIDs(chatList) { let fetchedReports; @@ -677,7 +697,7 @@ function unsubscribeFromReportChannel(reportID) { * * @param {String[]} participants * @param {Boolean} shouldNavigate - * @returns {Promise} + * @returns {Promise} */ function fetchOrCreateChatReport(participants, shouldNavigate = true) { if (participants.length < 2) { @@ -700,7 +720,7 @@ function fetchOrCreateChatReport(participants, shouldNavigate = true) { // Redirect the logged in person to the new report Navigation.navigate(ROUTES.getReportRoute(data.reportID)); } - return data.reportID; + return [data.reportID]; }); } @@ -753,8 +773,6 @@ function fetchAllReports( shouldRecordHomePageTiming = false, shouldDelayActionsFetch = false, ) { - let reportIDs = []; - API.Get({ returnValueList: 'chatList', }) @@ -764,7 +782,7 @@ function fetchAllReports( } // The cast here is necessary as Get rvl='chatList' may return an int or Array - reportIDs = String(response.chatList) + const reportIDs = String(response.chatList) .split(',') .filter(_.identity); @@ -773,12 +791,9 @@ function fetchAllReports( return fetchChatReportsByIDs(reportIDs); } - return fetchOrCreateChatReport([currentUserEmail, 'concierge@expensify.com'], false) - .then((createdReportID) => { - reportIDs = [createdReportID]; - }); + return fetchOrCreateChatReport([currentUserEmail, 'concierge@expensify.com'], false); }) - .then(() => { + .then((returnedReportIDs) => { Onyx.set(ONYXKEYS.INITIAL_REPORT_DATA_LOADED, true); if (shouldRecordHomePageTiming) { @@ -786,15 +801,27 @@ function fetchAllReports( } // Optionally delay fetching report history as it significantly increases sign in to interactive time - _.delay(() => { - Log.info('[Report] Fetching report actions for reports', true, {reportIDs}); - _.each(reportIDs, (reportID) => { - fetchActions(reportID); - }); - - // We are waiting 8 seconds since this provides a good time window to allow the UI to finish loading before - // bogging it down with more requests and operations. - }, shouldDelayActionsFetch ? 8000 : 0); + Timers.register(setTimeout(() => { + // Filter reports to see which ones have actions we need to fetch so we can preload Onyx with new + // content and improve chat switching experience + const reportIDsToFetchActions = _.filter(returnedReportIDs, id => ( + _.isUndefined(mostRecentStoredSequenceNumbers[id]) + || reportMaxSequenceNumbers[id] !== mostRecentStoredSequenceNumbers[id] + )); + + if (!_.isEmpty(reportIDsToFetchActions)) { + Log.info('[Report] Fetching report actions for reports', true, {reportIDsToFetchActions}); + _.each(reportIDsToFetchActions, (reportID) => { + const offset = mostRecentStoredSequenceNumbers[reportID]; + fetchActions(reportID, offset); + }); + } else { + Log.info('[Report] Local reportActions up to date. Not fetching additional actions.', true); + } + + // We are waiting 8 seconds since this provides a good time window to allow the UI to finish loading + // before bogging it down with more requests and operations. + }, shouldDelayActionsFetch ? 8000 : 1000)); }); } diff --git a/src/libs/actions/Session.js b/src/libs/actions/Session.js index fe01ed155a0a..0d95ba701924 100644 --- a/src/libs/actions/Session.js +++ b/src/libs/actions/Session.js @@ -8,6 +8,7 @@ import CONFIG from '../../CONFIG'; import PushNotification from '../Notification/PushNotification'; import Timing from './Timing'; import CONST from '../../CONST'; +import Timers from '../Timers'; let credentials = {}; Onyx.connect({ @@ -55,6 +56,7 @@ function createAccount(login) { */ function signOut() { Timing.clearData(); + Timers.clearAll(); redirectToSignIn(); console.debug('Redirecting to Sign In because signOut() was called'); diff --git a/src/libs/actions/SignInRedirect.js b/src/libs/actions/SignInRedirect.js index 4865af9ce614..da71f6fa9f52 100644 --- a/src/libs/actions/SignInRedirect.js +++ b/src/libs/actions/SignInRedirect.js @@ -1,7 +1,6 @@ import Onyx from 'react-native-onyx'; import ONYXKEYS from '../../ONYXKEYS'; import * as Pusher from '../Pusher/pusher'; -import NetworkConnection from '../NetworkConnection'; import UnreadIndicatorUpdater from '../UnreadIndicatorUpdater'; import PushNotification from '../Notification/PushNotification'; @@ -26,7 +25,6 @@ Onyx.connect({ * @param {String} [errorMessage] error message to be displayed on the sign in page */ function redirectToSignIn(errorMessage) { - NetworkConnection.stopListeningForReconnect(); UnreadIndicatorUpdater.stopListeningForReportChanges(); PushNotification.deregister(); Pusher.disconnect(); @@ -35,11 +33,6 @@ function redirectToSignIn(errorMessage) { return; } - // If we are already on the signin page, don't redirect - if (currentURL.indexOf('signin') !== -1) { - return; - } - const activeClients = currentActiveClients; // We must set the authToken to null so we can navigate to "signin" it's not possible to navigate to the route as From 20735f6805eb78edc1a02646e53cfeb3a23c5b4e Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Thu, 29 Apr 2021 17:10:13 -1000 Subject: [PATCH 02/13] add clarifying comments --- src/libs/actions/Report.js | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/src/libs/actions/Report.js b/src/libs/actions/Report.js index 744cc3ba9f36..1081222ea3f7 100644 --- a/src/libs/actions/Report.js +++ b/src/libs/actions/Report.js @@ -56,7 +56,13 @@ Onyx.connect({ }, }); -const mostRecentStoredSequenceNumbers = {}; +/** + * Map of the most recent non-loading report history items we have stored for a report. Not to be confused with the + * reportMaxSequenceNumbers. We can compare these values to those to determine whether there is "available history" + * to fetch from the server. maxSequenceNumber is retrieved when getting the reportList whereas these keep track of + * the actual reportActions saved in Onyx. + */ +const maxReportActionsSequenceNumbers = {}; Onyx.connect({ key: ONYXKEYS.COLLECTION.REPORT_ACTIONS, callback: (val, key) => { @@ -65,12 +71,17 @@ Onyx.connect({ } const reportID = CollectionUtils.extractCollectionItemID(key); - const mostRecentAction = CollectionUtils.lastItem(val); + const mostRecentAction = _.chain(val) + .filter(action => !action.loading) + .sortBy('sequenceNumber') + .last() + .value(); + if (!mostRecentAction || _.isUndefined(mostRecentAction.sequenceNumber)) { return; } - mostRecentStoredSequenceNumbers[reportID] = mostRecentAction.sequenceNumber; + maxReportActionsSequenceNumbers[reportID] = mostRecentAction.sequenceNumber; }, }); @@ -805,14 +816,14 @@ function fetchAllReports( // Filter reports to see which ones have actions we need to fetch so we can preload Onyx with new // content and improve chat switching experience const reportIDsToFetchActions = _.filter(returnedReportIDs, id => ( - _.isUndefined(mostRecentStoredSequenceNumbers[id]) - || reportMaxSequenceNumbers[id] !== mostRecentStoredSequenceNumbers[id] + _.isUndefined(maxReportActionsSequenceNumbers[id]) + || reportMaxSequenceNumbers[id] !== maxReportActionsSequenceNumbers[id] )); if (!_.isEmpty(reportIDsToFetchActions)) { Log.info('[Report] Fetching report actions for reports', true, {reportIDsToFetchActions}); _.each(reportIDsToFetchActions, (reportID) => { - const offset = mostRecentStoredSequenceNumbers[reportID]; + const offset = maxReportActionsSequenceNumbers[reportID]; fetchActions(reportID, offset); }); } else { From c6f5cbaa626d3cb18d9017e14a65f900b7fa4ef1 Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Thu, 29 Apr 2021 17:12:11 -1000 Subject: [PATCH 03/13] Fix up diffs --- src/libs/actions/Report.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/libs/actions/Report.js b/src/libs/actions/Report.js index 1081222ea3f7..e6fcdc7494ca 100644 --- a/src/libs/actions/Report.js +++ b/src/libs/actions/Report.js @@ -812,7 +812,7 @@ function fetchAllReports( } // Optionally delay fetching report history as it significantly increases sign in to interactive time - Timers.register(setTimeout(() => { + const timerID = setTimeout(() => { // Filter reports to see which ones have actions we need to fetch so we can preload Onyx with new // content and improve chat switching experience const reportIDsToFetchActions = _.filter(returnedReportIDs, id => ( @@ -832,7 +832,10 @@ function fetchAllReports( // We are waiting 8 seconds since this provides a good time window to allow the UI to finish loading // before bogging it down with more requests and operations. - }, shouldDelayActionsFetch ? 8000 : 1000)); + }, shouldDelayActionsFetch ? 8000 : 1000); + + // Register the timer so we can clean it up on sign out if it has not yet fired. + Timers.register(timerID); }); } From fee879557fd117264ad1527e26e0171dfb189326 Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Thu, 29 Apr 2021 17:16:48 -1000 Subject: [PATCH 04/13] move timer clear to redirectToSignin --- src/libs/actions/Session.js | 2 -- src/libs/actions/SignInRedirect.js | 2 ++ 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libs/actions/Session.js b/src/libs/actions/Session.js index 0d95ba701924..fe01ed155a0a 100644 --- a/src/libs/actions/Session.js +++ b/src/libs/actions/Session.js @@ -8,7 +8,6 @@ import CONFIG from '../../CONFIG'; import PushNotification from '../Notification/PushNotification'; import Timing from './Timing'; import CONST from '../../CONST'; -import Timers from '../Timers'; let credentials = {}; Onyx.connect({ @@ -56,7 +55,6 @@ function createAccount(login) { */ function signOut() { Timing.clearData(); - Timers.clearAll(); redirectToSignIn(); console.debug('Redirecting to Sign In because signOut() was called'); diff --git a/src/libs/actions/SignInRedirect.js b/src/libs/actions/SignInRedirect.js index da71f6fa9f52..9e7b787afbe1 100644 --- a/src/libs/actions/SignInRedirect.js +++ b/src/libs/actions/SignInRedirect.js @@ -3,6 +3,7 @@ import ONYXKEYS from '../../ONYXKEYS'; import * as Pusher from '../Pusher/pusher'; import UnreadIndicatorUpdater from '../UnreadIndicatorUpdater'; import PushNotification from '../Notification/PushNotification'; +import Timers from '../Timers'; let currentURL; Onyx.connect({ @@ -28,6 +29,7 @@ function redirectToSignIn(errorMessage) { UnreadIndicatorUpdater.stopListeningForReportChanges(); PushNotification.deregister(); Pusher.disconnect(); + Timers.clearAll(); if (!currentURL) { return; From 1df9d3ec0849250394918df921d4b8fca18ad467 Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Thu, 29 Apr 2021 17:42:35 -1000 Subject: [PATCH 05/13] add more comments --- src/CONST.js | 4 ++++ src/libs/actions/Report.js | 19 +++++++++++++------ 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/src/CONST.js b/src/CONST.js index b3b873a398c0..8eff75e18f4f 100644 --- a/src/CONST.js +++ b/src/CONST.js @@ -125,6 +125,10 @@ const CONST = { STAGING: 'STG', PRODUCTION: 'PROD', }, + DELAY: { + STARTUP: 8000, + RECONNECT: 1000, + }, }; export default CONST; diff --git a/src/libs/actions/Report.js b/src/libs/actions/Report.js index e6fcdc7494ca..89d91416b0dc 100644 --- a/src/libs/actions/Report.js +++ b/src/libs/actions/Report.js @@ -811,13 +811,19 @@ function fetchAllReports( Timing.end(CONST.TIMING.HOMEPAGE_REPORTS_LOADED); } - // Optionally delay fetching report history as it significantly increases sign in to interactive time + // Optionally delay fetching report history as it significantly increases sign in to interactive time. const timerID = setTimeout(() => { // Filter reports to see which ones have actions we need to fetch so we can preload Onyx with new - // content and improve chat switching experience + // content and improve chat switching experience by only downloading content we don't have yet. + // This improves performance significantly when reconnecting by limiting API requests and unnecessary + // data processing by Onyx. const reportIDsToFetchActions = _.filter(returnedReportIDs, id => ( _.isUndefined(maxReportActionsSequenceNumbers[id]) - || reportMaxSequenceNumbers[id] !== maxReportActionsSequenceNumbers[id] + + // The most recent reportAction we have stored is less than the maxSequenceNumber for a report. + // This works because the reportMaxSequenceNumber is based on the reportActionList whereas the + // maxReportActionsSequenceNumbers can only be populated by adding reportActions to Onyx. + || maxReportActionsSequenceNumbers[id] < reportMaxSequenceNumbers[id] )); if (!_.isEmpty(reportIDsToFetchActions)) { @@ -830,9 +836,10 @@ function fetchAllReports( Log.info('[Report] Local reportActions up to date. Not fetching additional actions.', true); } - // We are waiting 8 seconds since this provides a good time window to allow the UI to finish loading - // before bogging it down with more requests and operations. - }, shouldDelayActionsFetch ? 8000 : 1000); + // We are waiting a set amount of time to allow the UI to finish loading before bogging it down with + // more requests and operations. Startup delay is longer since there is a lot more work done to build + // up the UI when the app first initializes. + }, shouldDelayActionsFetch ? CONST.DELAY.STARTUP : CONST.DELAY.RECONNECT); // Register the timer so we can clean it up on sign out if it has not yet fired. Timers.register(timerID); From 09e2a9ebe5d6ee613791bcbb6c19c29302b86b02 Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Fri, 30 Apr 2021 07:32:51 -1000 Subject: [PATCH 06/13] Improve setting most recent action sequence number. Add comments. --- src/libs/CollectionUtils.js | 2 +- src/libs/Timers.js | 2 +- src/libs/actions/Report.js | 16 ++++++++-------- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/libs/CollectionUtils.js b/src/libs/CollectionUtils.js index e1f5faead1ba..d51e7adad5e8 100644 --- a/src/libs/CollectionUtils.js +++ b/src/libs/CollectionUtils.js @@ -6,7 +6,7 @@ import _ from 'underscore'; * e.g. {1: '1', 2: '2', 3: '3'} -> '3' * * @param {Object} object - * @return {*} + * @returns {*} */ function lastItem(object = {}) { const lastKey = _.last(_.keys(object)) || 0; diff --git a/src/libs/Timers.js b/src/libs/Timers.js index 560ea154d169..49bc6a7350b8 100644 --- a/src/libs/Timers.js +++ b/src/libs/Timers.js @@ -6,7 +6,7 @@ const timers = []; * Register a timer so it can be cleaned up later. * * @param {Number} timerID - * @return {Number} + * @returns {Number} */ function register(timerID) { timers.push(timerID); diff --git a/src/libs/actions/Report.js b/src/libs/actions/Report.js index 89d91416b0dc..9208ca9677c2 100644 --- a/src/libs/actions/Report.js +++ b/src/libs/actions/Report.js @@ -65,18 +65,15 @@ Onyx.connect({ const maxReportActionsSequenceNumbers = {}; Onyx.connect({ key: ONYXKEYS.COLLECTION.REPORT_ACTIONS, - callback: (val, key) => { - if (!key || !val) { + callback: (actions, key) => { + if (!key || !actions) { return; } const reportID = CollectionUtils.extractCollectionItemID(key); - const mostRecentAction = _.chain(val) - .filter(action => !action.loading) - .sortBy('sequenceNumber') - .last() - .value(); - + const actionsArray = _.toArray(actions); + const mostRecentNonLoadingActionIndex = _.findLastIndex(actionsArray, action => !action.loading); + const mostRecentAction = actionsArray[mostRecentNonLoadingActionIndex]; if (!mostRecentAction || _.isUndefined(mostRecentAction.sequenceNumber)) { return; } @@ -731,6 +728,9 @@ function fetchOrCreateChatReport(participants, shouldNavigate = true) { // Redirect the logged in person to the new report Navigation.navigate(ROUTES.getReportRoute(data.reportID)); } + + // We are returning an array with the reportID here since fetchAllReports calls this method or + // fetchChatReportsByIDs which returns an array of reportIDs. return [data.reportID]; }); } From bc4a772d232065d05107827184ed41af4d28727e Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Fri, 30 Apr 2021 09:03:50 -1000 Subject: [PATCH 07/13] Add more verbose comments --- src/CONST.js | 6 +++- src/libs/actions/Report.js | 69 +++++++++++++++++++++++--------------- 2 files changed, 47 insertions(+), 28 deletions(-) diff --git a/src/CONST.js b/src/CONST.js index 8eff75e18f4f..1de6d071ee65 100644 --- a/src/CONST.js +++ b/src/CONST.js @@ -125,7 +125,11 @@ const CONST = { STAGING: 'STG', PRODUCTION: 'PROD', }, - DELAY: { + + // Used to delay the initial fetching of reportActions when the app first inits or reconnects (e.g. returning + // from backgound). The times are based on how long it generally seems to take for the app to become interactive + // in each scenario. + FETCH_ACTIONS_DELAY: { STARTUP: 8000, RECONNECT: 1000, }, diff --git a/src/libs/actions/Report.js b/src/libs/actions/Report.js index 9208ca9677c2..90d441d63822 100644 --- a/src/libs/actions/Report.js +++ b/src/libs/actions/Report.js @@ -56,13 +56,37 @@ Onyx.connect({ }, }); +const typingWatchTimers = {}; + +/** + * Map of the most recent sequenceNumber for a reports_* key in Onyx by reportID. + * + * There are several sources that can set the most recent reportAction's sequenceNumber for a report: + * + * - Fetching the report object + * - Fetching the report history + * - Optimistically creating a report action + * - Handling a report action via Pusher + * + * Those values are stored in reportMaxSequenceNumbers and treated as the main source of truth about what a report's max + * sequenceNumber is. + */ +const reportMaxSequenceNumbers = {}; + /** - * Map of the most recent non-loading report history items we have stored for a report. Not to be confused with the - * reportMaxSequenceNumbers. We can compare these values to those to determine whether there is "available history" - * to fetch from the server. maxSequenceNumber is retrieved when getting the reportList whereas these keep track of - * the actual reportActions saved in Onyx. + * Map of the most recent non-loading sequenceNumber for a reportActions_* key in Onyx by reportID. + * + * What's the difference between reportMaxSequenceNumbers and UNSAFE_reportActionsMaxSequenceNumbers? + * + * Knowing the maxSequenceNumber for a report does not necessarily mean we have stored the report actions for that + * report. To optimize and understand which reportActions we need to fetch we also keep track of the max sequenceNumber + * for the stored reportActions in UNSAFE_reportActionsMaxSequenceNumbers. This allows us to initially download all + * reportActions when the app starts up and then only download the actions that we need when the app reconnects. + * + * We prefix this with UNSAFE to communicate that this should only be used in the correct contexts. In most cases, + * reportMaxSequenceNumbers should be referenced and not the locally stored reportAction's max sequenceNumber. */ -const maxReportActionsSequenceNumbers = {}; +const UNSAFE_reportActionsMaxSequenceNumbers = {}; Onyx.connect({ key: ONYXKEYS.COLLECTION.REPORT_ACTIONS, callback: (actions, key) => { @@ -78,15 +102,10 @@ Onyx.connect({ return; } - maxReportActionsSequenceNumbers[reportID] = mostRecentAction.sequenceNumber; + UNSAFE_reportActionsMaxSequenceNumbers[reportID] = mostRecentAction.sequenceNumber; }, }); -const typingWatchTimers = {}; - -// Keeps track of the max sequence number for each report -const reportMaxSequenceNumbers = {}; - // Keeps track of the last read for each report const lastReadSequenceNumbers = {}; @@ -818,30 +837,26 @@ function fetchAllReports( // This improves performance significantly when reconnecting by limiting API requests and unnecessary // data processing by Onyx. const reportIDsToFetchActions = _.filter(returnedReportIDs, id => ( - _.isUndefined(maxReportActionsSequenceNumbers[id]) - - // The most recent reportAction we have stored is less than the maxSequenceNumber for a report. - // This works because the reportMaxSequenceNumber is based on the reportActionList whereas the - // maxReportActionsSequenceNumbers can only be populated by adding reportActions to Onyx. - || maxReportActionsSequenceNumbers[id] < reportMaxSequenceNumbers[id] + _.isUndefined(UNSAFE_reportActionsMaxSequenceNumbers[id]) + || UNSAFE_reportActionsMaxSequenceNumbers[id] < reportMaxSequenceNumbers[id] )); - if (!_.isEmpty(reportIDsToFetchActions)) { - Log.info('[Report] Fetching report actions for reports', true, {reportIDsToFetchActions}); - _.each(reportIDsToFetchActions, (reportID) => { - const offset = maxReportActionsSequenceNumbers[reportID]; - fetchActions(reportID, offset); - }); - } else { - Log.info('[Report] Local reportActions up to date. Not fetching additional actions.', true); + if (_.isEmpty(reportIDsToFetchActions)) { + return; } + _.each(reportIDsToFetchActions, (reportID) => { + const offset = UNSAFE_reportActionsMaxSequenceNumbers[reportID]; + fetchActions(reportID, offset); + }); + // We are waiting a set amount of time to allow the UI to finish loading before bogging it down with // more requests and operations. Startup delay is longer since there is a lot more work done to build // up the UI when the app first initializes. - }, shouldDelayActionsFetch ? CONST.DELAY.STARTUP : CONST.DELAY.RECONNECT); + }, shouldDelayActionsFetch ? CONST.FETCH_ACTIONS_DELAY.STARTUP : CONST.FETCH_ACTIONS_DELAY.RECONNECT); - // Register the timer so we can clean it up on sign out if it has not yet fired. + // Register the timer so we can clean it up if the user logs out after logging in. If we don't cancel the + // timer we'll make unnecessary API requests from the sign in page. Timers.register(timerID); }); } From 65e8be46806ba02cdc4fed0a637d996cd48c9c30 Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Fri, 30 Apr 2021 11:33:00 -1000 Subject: [PATCH 08/13] Move the map to its own file and discourage use --- src/libs/actions/Report.js | 40 ++--------------- src/libs/actions/ReportActions.js | 72 +++++++++++++++++++++++++++++++ 2 files changed, 75 insertions(+), 37 deletions(-) create mode 100644 src/libs/actions/ReportActions.js diff --git a/src/libs/actions/Report.js b/src/libs/actions/Report.js index 90d441d63822..7b790358684f 100644 --- a/src/libs/actions/Report.js +++ b/src/libs/actions/Report.js @@ -18,8 +18,8 @@ import * as API from '../API'; import CONST from '../../CONST'; import Log from '../Log'; import {isReportMessageAttachment} from '../reportUtils'; -import * as CollectionUtils from '../CollectionUtils'; import Timers from '../Timers'; +import {dangerouslyGetReportActionsMaxSequenceNumber, isReportMissingActions} from './ReportActions'; let currentUserEmail; let currentUserAccountID; @@ -73,39 +73,6 @@ const typingWatchTimers = {}; */ const reportMaxSequenceNumbers = {}; -/** - * Map of the most recent non-loading sequenceNumber for a reportActions_* key in Onyx by reportID. - * - * What's the difference between reportMaxSequenceNumbers and UNSAFE_reportActionsMaxSequenceNumbers? - * - * Knowing the maxSequenceNumber for a report does not necessarily mean we have stored the report actions for that - * report. To optimize and understand which reportActions we need to fetch we also keep track of the max sequenceNumber - * for the stored reportActions in UNSAFE_reportActionsMaxSequenceNumbers. This allows us to initially download all - * reportActions when the app starts up and then only download the actions that we need when the app reconnects. - * - * We prefix this with UNSAFE to communicate that this should only be used in the correct contexts. In most cases, - * reportMaxSequenceNumbers should be referenced and not the locally stored reportAction's max sequenceNumber. - */ -const UNSAFE_reportActionsMaxSequenceNumbers = {}; -Onyx.connect({ - key: ONYXKEYS.COLLECTION.REPORT_ACTIONS, - callback: (actions, key) => { - if (!key || !actions) { - return; - } - - const reportID = CollectionUtils.extractCollectionItemID(key); - const actionsArray = _.toArray(actions); - const mostRecentNonLoadingActionIndex = _.findLastIndex(actionsArray, action => !action.loading); - const mostRecentAction = actionsArray[mostRecentNonLoadingActionIndex]; - if (!mostRecentAction || _.isUndefined(mostRecentAction.sequenceNumber)) { - return; - } - - UNSAFE_reportActionsMaxSequenceNumbers[reportID] = mostRecentAction.sequenceNumber; - }, -}); - // Keeps track of the last read for each report const lastReadSequenceNumbers = {}; @@ -837,8 +804,7 @@ function fetchAllReports( // This improves performance significantly when reconnecting by limiting API requests and unnecessary // data processing by Onyx. const reportIDsToFetchActions = _.filter(returnedReportIDs, id => ( - _.isUndefined(UNSAFE_reportActionsMaxSequenceNumbers[id]) - || UNSAFE_reportActionsMaxSequenceNumbers[id] < reportMaxSequenceNumbers[id] + isReportMissingActions(id, reportMaxSequenceNumbers[id]) )); if (_.isEmpty(reportIDsToFetchActions)) { @@ -846,7 +812,7 @@ function fetchAllReports( } _.each(reportIDsToFetchActions, (reportID) => { - const offset = UNSAFE_reportActionsMaxSequenceNumbers[reportID]; + const offset = dangerouslyGetReportActionsMaxSequenceNumber(reportID, false); fetchActions(reportID, offset); }); diff --git a/src/libs/actions/ReportActions.js b/src/libs/actions/ReportActions.js new file mode 100644 index 000000000000..4ec2584359c9 --- /dev/null +++ b/src/libs/actions/ReportActions.js @@ -0,0 +1,72 @@ +import _ from 'underscore'; +import Onyx from 'react-native-onyx'; +import ONYXKEYS from '../../ONYXKEYS'; +import * as CollectionUtils from '../CollectionUtils'; + +/** + * Map of the most recent non-loading sequenceNumber for a reportActions_* key in Onyx by reportID. + * + * What's the difference between reportMaxSequenceNumbers and reportActionsMaxSequenceNumbers? + * + * Knowing the maxSequenceNumber for a report does not necessarily mean we have stored the report actions for that + * report. To optimize and understand which reportActions we need to fetch we also keep track of the max sequenceNumber + * for the stored reportActions in reportActionsMaxSequenceNumbers. This allows us to initially download all + * reportActions when the app starts up and then only download the actions that we need when the app reconnects. + * + * This information should only be used in the correct contexts. In most cases, reportMaxSequenceNumbers should be + * referenced and not the locally stored reportAction's max sequenceNumber. + */ +const reportActionsMaxSequenceNumbers = {}; +Onyx.connect({ + key: ONYXKEYS.COLLECTION.REPORT_ACTIONS, + callback: (actions, key) => { + if (!key || !actions) { + return; + } + + const reportID = CollectionUtils.extractCollectionItemID(key); + const actionsArray = _.toArray(actions); + const mostRecentNonLoadingActionIndex = _.findLastIndex(actionsArray, action => !action.loading); + const mostRecentAction = actionsArray[mostRecentNonLoadingActionIndex]; + if (!mostRecentAction || _.isUndefined(mostRecentAction.sequenceNumber)) { + return; + } + + reportActionsMaxSequenceNumbers[reportID] = mostRecentAction.sequenceNumber; + }, +}); + +/** + * WARNING: Do not use this method to access the maxSequenceNumber for a report. This ONLY returns the maxSequenceNumber + * for reportActions that are stored in Onyx under a reportActions_* key. + * + * @param {Number} reportID + * @param {Boolean} shouldWarn + * @returns {Number} + */ +function dangerouslyGetReportActionsMaxSequenceNumber(reportID, shouldWarn = true) { + if (shouldWarn) { + // eslint-disable-next-line max-len + console.error('dangerouslyGetReportActionsMaxSequenceNumber is unreliable and should not be used to access the maxSequenceNumber for a report. Use reportMaxSequenceNumbers[reportID] instead.'); + } + + return reportActionsMaxSequenceNumbers[reportID]; +} + +/** + * Compares the maximum sequenceNumber that we know about with the most recent report action we have saved. + * If we have no report actions at all for the report we will assume that it is missing actions. + * + * @param {Number} reportID + * @param {Number} maxKnownSequenceNumber + * @returns {Boolean} + */ +function isReportMissingActions(reportID, maxKnownSequenceNumber) { + return _.isUndefined(reportActionsMaxSequenceNumbers[reportID]) + || reportActionsMaxSequenceNumbers[reportID] < maxKnownSequenceNumber; +} + +export { + isReportMissingActions, + dangerouslyGetReportActionsMaxSequenceNumber, +}; From 2dd4ccd7566bcb0f3b8e47bc3f10096b01424d26 Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Fri, 30 Apr 2021 11:40:28 -1000 Subject: [PATCH 09/13] make warning scarier --- src/libs/actions/ReportActions.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libs/actions/ReportActions.js b/src/libs/actions/ReportActions.js index 4ec2584359c9..c4191fec604e 100644 --- a/src/libs/actions/ReportActions.js +++ b/src/libs/actions/ReportActions.js @@ -47,7 +47,7 @@ Onyx.connect({ function dangerouslyGetReportActionsMaxSequenceNumber(reportID, shouldWarn = true) { if (shouldWarn) { // eslint-disable-next-line max-len - console.error('dangerouslyGetReportActionsMaxSequenceNumber is unreliable and should not be used to access the maxSequenceNumber for a report. Use reportMaxSequenceNumbers[reportID] instead.'); + console.error('WARNING: dangerouslyGetReportActionsMaxSequenceNumber is unreliable as it ONLY references reportActions in storage. It should not be used to access the maxSequenceNumber for a report. Use reportMaxSequenceNumbers[reportID] instead.'); } return reportActionsMaxSequenceNumbers[reportID]; From 9c20efd21476abc5d8e7ddffb2e86b12c5acaa17 Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Fri, 30 Apr 2021 11:55:38 -1000 Subject: [PATCH 10/13] add logs back as debugs --- src/libs/actions/Report.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/libs/actions/Report.js b/src/libs/actions/Report.js index 7b790358684f..0c3f1737076d 100644 --- a/src/libs/actions/Report.js +++ b/src/libs/actions/Report.js @@ -808,10 +808,14 @@ function fetchAllReports( )); if (_.isEmpty(reportIDsToFetchActions)) { + console.debug('[Report] Local reportActions up to date. Not fetching additional actions.'); return; } _.each(reportIDsToFetchActions, (reportID) => { + console.debug('[Report] Fetching reportActions for reportIDs: ', { + reportIDs: reportIDsToFetchActions, + }); const offset = dangerouslyGetReportActionsMaxSequenceNumber(reportID, false); fetchActions(reportID, offset); }); From 4f3680edcdba9a3866968abaf38693b512db03ce Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Fri, 30 Apr 2021 12:02:41 -1000 Subject: [PATCH 11/13] move log --- src/libs/actions/Report.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libs/actions/Report.js b/src/libs/actions/Report.js index 0c3f1737076d..c2371297dd5a 100644 --- a/src/libs/actions/Report.js +++ b/src/libs/actions/Report.js @@ -812,10 +812,10 @@ function fetchAllReports( return; } + console.debug('[Report] Fetching reportActions for reportIDs: ', { + reportIDs: reportIDsToFetchActions, + }); _.each(reportIDsToFetchActions, (reportID) => { - console.debug('[Report] Fetching reportActions for reportIDs: ', { - reportIDs: reportIDsToFetchActions, - }); const offset = dangerouslyGetReportActionsMaxSequenceNumber(reportID, false); fetchActions(reportID, offset); }); From 9f2e5723efa0a66a9df8a46ac6a4837c5e6b9d13 Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Fri, 30 Apr 2021 12:06:46 -1000 Subject: [PATCH 12/13] Split error onto several lines --- src/libs/actions/ReportActions.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/libs/actions/ReportActions.js b/src/libs/actions/ReportActions.js index c4191fec604e..7d846ac6abeb 100644 --- a/src/libs/actions/ReportActions.js +++ b/src/libs/actions/ReportActions.js @@ -46,8 +46,9 @@ Onyx.connect({ */ function dangerouslyGetReportActionsMaxSequenceNumber(reportID, shouldWarn = true) { if (shouldWarn) { - // eslint-disable-next-line max-len - console.error('WARNING: dangerouslyGetReportActionsMaxSequenceNumber is unreliable as it ONLY references reportActions in storage. It should not be used to access the maxSequenceNumber for a report. Use reportMaxSequenceNumbers[reportID] instead.'); + console.error('WARNING: dangerouslyGetReportActionsMaxSequenceNumber is unreliable as it ONLY references ' + + 'reportActions in storage. It should not be used to access the maxSequenceNumber for a report. Use ' + + 'reportMaxSequenceNumbers[reportID] instead.'); } return reportActionsMaxSequenceNumbers[reportID]; From 135ab15684f71a88c29ed085b3ca6e1d997c88ca Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Mon, 3 May 2021 08:20:14 -1000 Subject: [PATCH 13/13] Make requested changes --- src/libs/actions/Report.js | 16 +++++++--------- src/libs/actions/ReportActions.js | 4 ++-- 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/src/libs/actions/Report.js b/src/libs/actions/Report.js index c2371297dd5a..4860af2cfe60 100644 --- a/src/libs/actions/Report.js +++ b/src/libs/actions/Report.js @@ -68,8 +68,8 @@ const typingWatchTimers = {}; * - Optimistically creating a report action * - Handling a report action via Pusher * - * Those values are stored in reportMaxSequenceNumbers and treated as the main source of truth about what a report's max - * sequenceNumber is. + * Those values are stored in reportMaxSequenceNumbers and treated as the main source of truth for each report's max + * sequenceNumber. */ const reportMaxSequenceNumbers = {}; @@ -797,8 +797,10 @@ function fetchAllReports( Timing.end(CONST.TIMING.HOMEPAGE_REPORTS_LOADED); } - // Optionally delay fetching report history as it significantly increases sign in to interactive time. - const timerID = setTimeout(() => { + // Delay fetching report history as it significantly increases sign in to interactive time. + // Register the timer so we can clean it up if the user quickly logs out after logging in. If we don't + // cancel the timer we'll make unnecessary API requests from the sign in page. + Timers.register(setTimeout(() => { // Filter reports to see which ones have actions we need to fetch so we can preload Onyx with new // content and improve chat switching experience by only downloading content we don't have yet. // This improves performance significantly when reconnecting by limiting API requests and unnecessary @@ -823,11 +825,7 @@ function fetchAllReports( // We are waiting a set amount of time to allow the UI to finish loading before bogging it down with // more requests and operations. Startup delay is longer since there is a lot more work done to build // up the UI when the app first initializes. - }, shouldDelayActionsFetch ? CONST.FETCH_ACTIONS_DELAY.STARTUP : CONST.FETCH_ACTIONS_DELAY.RECONNECT); - - // Register the timer so we can clean it up if the user logs out after logging in. If we don't cancel the - // timer we'll make unnecessary API requests from the sign in page. - Timers.register(timerID); + }, shouldDelayActionsFetch ? CONST.FETCH_ACTIONS_DELAY.STARTUP : CONST.FETCH_ACTIONS_DELAY.RECONNECT)); }); } diff --git a/src/libs/actions/ReportActions.js b/src/libs/actions/ReportActions.js index 7d846ac6abeb..74fbb2dfb83d 100644 --- a/src/libs/actions/ReportActions.js +++ b/src/libs/actions/ReportActions.js @@ -9,7 +9,7 @@ import * as CollectionUtils from '../CollectionUtils'; * What's the difference between reportMaxSequenceNumbers and reportActionsMaxSequenceNumbers? * * Knowing the maxSequenceNumber for a report does not necessarily mean we have stored the report actions for that - * report. To optimize and understand which reportActions we need to fetch we also keep track of the max sequenceNumber + * report. To understand and optimize which reportActions we need to fetch we also keep track of the max sequenceNumber * for the stored reportActions in reportActionsMaxSequenceNumbers. This allows us to initially download all * reportActions when the app starts up and then only download the actions that we need when the app reconnects. * @@ -64,7 +64,7 @@ function dangerouslyGetReportActionsMaxSequenceNumber(reportID, shouldWarn = tru */ function isReportMissingActions(reportID, maxKnownSequenceNumber) { return _.isUndefined(reportActionsMaxSequenceNumbers[reportID]) - || reportActionsMaxSequenceNumbers[reportID] < maxKnownSequenceNumber; + || reportActionsMaxSequenceNumbers[reportID] < maxKnownSequenceNumber; } export {