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

Send mostRecentReportActionLastModified with ReconnectApp #18807

Merged
merged 32 commits into from
Jul 4, 2023
Merged
Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
8ac9d8c
method to get most recent blahblah
marcaaron May 10, 2023
72dcafc
add getMostRecentReportActionLastModified()
marcaaron May 11, 2023
8299384
Fix conflicts
marcaaron May 11, 2023
3b680b0
Actually send the timestamp
marcaaron May 11, 2023
e176806
Subscribe to correct key
marcaaron May 12, 2023
ce9e668
fix conflict
marcaaron May 19, 2023
b0c0aae
Merge branch 'main' into marcaaron-addLastModified
marcaaron May 19, 2023
32e19d0
Run prettier
marcaaron May 19, 2023
c3bf45b
Merge branch 'main' into marcaaron-addLastModified
marcaaron May 31, 2023
d875a91
Merge branch 'main' into marcaaron-addLastModified
marcaaron Jun 1, 2023
bb88c62
check reports and policies
marcaaron Jun 13, 2023
0619651
Merge branch 'main' into marcaaron-addLastModified
marcaaron Jun 13, 2023
ec5a796
no app changes
marcaaron Jun 14, 2023
79feeef
Revert "no app changes"
marcaaron Jun 14, 2023
e1ddfc2
revert stuff
marcaaron Jun 14, 2023
2ac3528
revert more stuff
marcaaron Jun 14, 2023
6adf6de
More
marcaaron Jun 14, 2023
4df4678
account for possible undefined report value
marcaaron Jun 15, 2023
112b97d
update comment
marcaaron Jun 16, 2023
4eda351
Merge branch 'main' into marcaaron-addLastModified
marcaaron Jun 16, 2023
a88a3cb
Revert problem code
marcaaron Jun 16, 2023
c0583f8
DRY up openApp/reconnectApp method
marcaaron Jun 16, 2023
0942351
run prettier add js doc
marcaaron Jun 16, 2023
daf4e6f
Log an alert if we exceed max execution time
marcaaron Jun 20, 2023
827562e
Fix conflicts
marcaaron Jun 27, 2023
87b1c91
Apply suggested feedback
marcaaron Jun 27, 2023
c256bb4
Fix
marcaaron Jun 27, 2023
0a83f50
Fix dependency cycle
marcaaron Jun 27, 2023
eaca434
Merge branch 'main' into marcaaron-addLastModified
marcaaron Jun 28, 2023
76f76ad
Make requested changes
marcaaron Jun 28, 2023
41487c2
Fix conflicts
marcaaron Jul 3, 2023
2341952
prettier
marcaaron Jul 3, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions src/CONST.js
Original file line number Diff line number Diff line change
Expand Up @@ -652,6 +652,7 @@ const CONST = {
},
},
TIMING: {
CALCULATE_MOST_RECENT_LAST_MODIFIED_ACTION: 'calc_most_recent_last_modified_action',
SEARCH_RENDER: 'search_render',
HOMEPAGE_INITIAL_RENDER: 'homepage_initial_render',
REPORT_INITIAL_RENDER: 'report_initial_render',
Expand Down Expand Up @@ -724,9 +725,6 @@ const CONST = {
MAX_RETRY_WAIT_TIME_MS: 10 * 1000,
PROCESS_REQUEST_DELAY_MS: 1000,
MAX_PENDING_TIME_MS: 10 * 1000,
COMMAND: {
RECONNECT_APP: 'ReconnectApp',
},
},
DEFAULT_TIME_ZONE: {automatic: true, selected: 'America/Los_Angeles'},
DEFAULT_ACCOUNT_DATA: {errors: null, success: '', isLoading: false},
Expand Down
22 changes: 3 additions & 19 deletions src/libs/HttpUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,6 @@ Onyx.connect({
// We use the AbortController API to terminate pending request in `cancelPendingRequests`
let cancellationController = new AbortController();

// To terminate pending ReconnectApp requests https://github.com/Expensify/App/issues/15627
let reconnectAppCancellationController = new AbortController();

/**
* Send an HTTP request, and attempt to resolve the json response.
* If there is a network error, we'll set the application offline.
Expand All @@ -33,18 +30,12 @@ let reconnectAppCancellationController = new AbortController();
* @param {String} [method]
* @param {Object} [body]
* @param {Boolean} [canCancel]
* @param {String} [command]
* @returns {Promise}
*/
function processHTTPRequest(url, method = 'get', body = null, canCancel = true, command = '') {
let signal;
if (canCancel) {
signal = command === CONST.NETWORK.COMMAND.RECONNECT_APP ? reconnectAppCancellationController.signal : cancellationController.signal;
}

function processHTTPRequest(url, method = 'get', body = null, canCancel = true) {
return fetch(url, {
// We hook requests to the same Controller signal, so we can cancel them all at once
signal,
signal: canCancel ? cancellationController.signal : undefined,
method,
body,
})
Expand Down Expand Up @@ -136,12 +127,7 @@ function xhr(command, data, type = CONST.NETWORK.METHOD.POST, shouldUseSecure =
});

const url = ApiUtils.getCommandURL({shouldUseSecure, command});
return processHTTPRequest(url, type, formData, data.canCancel, command);
}

function cancelPendingReconnectAppRequest() {
reconnectAppCancellationController.abort();
reconnectAppCancellationController = new AbortController();
return processHTTPRequest(url, type, formData, data.canCancel);
}

function cancelPendingRequests() {
Expand All @@ -150,11 +136,9 @@ function cancelPendingRequests() {
// We create a new instance because once `abort()` is called any future requests using the same controller would
// automatically get rejected: https://dom.spec.whatwg.org/#abortcontroller-api-integration
cancellationController = new AbortController();
cancelPendingReconnectAppRequest();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you clarify why you removed this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

More context here: #18807 (comment)

Lmk if you want to discuss further? I am going to cause a regression with this - but the original solution is a hack.

}

export default {
xhr,
cancelPendingRequests,
cancelPendingReconnectAppRequest,
};
10 changes: 9 additions & 1 deletion src/libs/Navigation/AppNavigator/AuthScreens.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import CentralPaneNavigator from './Navigators/CentralPaneNavigator';
import NAVIGATORS from '../../../NAVIGATORS';
import FullScreenNavigator from './Navigators/FullScreenNavigator';
import styles from '../../../styles/styles';
import * as SessionUtils from '../../SessionUtils';

let currentUserEmail;
Onyx.connect({
Expand Down Expand Up @@ -119,7 +120,14 @@ class AuthScreens extends React.Component {
User.subscribeToUserEvents();
});

App.openApp();
// If we are on this screen then we are "logged in", but the user might not have "just logged in". They could be reopening the app
// or returning from background. If so, we'll assume they have some app data already and we can call reconnectApp() instead of openApp().
if (SessionUtils.didUserLogInDuringSession()) {
App.openApp();
} else {
App.reconnectApp();
}

App.setUpPoliciesAndNavigate(this.props.session);

if (this.props.lastOpenedPublicRoomID) {
Expand Down
57 changes: 57 additions & 0 deletions src/libs/ReportActionsUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,20 @@ import ONYXKEYS from '../ONYXKEYS';
import Log from './Log';
import * as CurrencyUtils from './CurrencyUtils';
import isReportMessageAttachment from './isReportMessageAttachment';
import Timing from './actions/Timing';

const allReports = {};
Onyx.connect({
key: ONYXKEYS.COLLECTION.REPORT,
callback: (report, key) => {
if (!key || !report) {
return;
}

const reportID = CollectionUtils.extractCollectionItemID(key);
allReports[reportID] = report;
},
});

const allReportActions = {};
Onyx.connect({
Expand Down Expand Up @@ -409,6 +423,48 @@ function getLinkedTransactionID(reportID, reportActionID) {
return reportAction.originalMessage.IOUTransactionID;
}

/**
* @returns {string}
*/
function getMostRecentReportActionLastModified() {
Timing.start(CONST.TIMING.CALCULATE_MOST_RECENT_LAST_MODIFIED_ACTION);

// Start with the oldest date possible
let mostRecentReportActionLastModified = new Date(0).toISOString();

// Flatten all the actions
// Loop over them all to find the one that is the most recent
marcaaron marked this conversation as resolved.
Show resolved Hide resolved
const flatReportActions = _.flatten(_.map(allReportActions, (actions) => _.values(actions)));
marcaaron marked this conversation as resolved.
Show resolved Hide resolved
_.each(flatReportActions, (action) => {
// Pending actions should not be counted here as a user could create a comment or some other action while offline and the server might know about
// messages they have not seen yet.
if (!_.isEmpty(action.pendingAction)) {
return;
}

const lastModified = action.lastModified || action.created;
if (lastModified < mostRecentReportActionLastModified) {
return;
}

mostRecentReportActionLastModified = lastModified;
});

// We might not have actions so we also look at the report objects to see if any have a lastVisibleActionLastModified that is more recent. We don't need to get
// any reports that have been updated before either a recently updated report or reportAction as we should be up to date on these
_.each(allReports, (report) => {
const reportLastVisibleActionLastModified = report.lastVisibleActionLastModified || report.lastVisibleActionCreated;
if (!reportLastVisibleActionLastModified || reportLastVisibleActionLastModified < mostRecentReportActionLastModified) {
return;
}

mostRecentReportActionLastModified = reportLastVisibleActionLastModified;
});

Timing.end(CONST.TIMING.CALCULATE_MOST_RECENT_LAST_MODIFIED_ACTION, 500);
marcaaron marked this conversation as resolved.
Show resolved Hide resolved
return mostRecentReportActionLastModified;
}

/**
* @param {*} chatReportID
* @param {*} iouReportID
Expand Down Expand Up @@ -455,6 +511,7 @@ export {
isMoneyRequestAction,
hasCommentThread,
getLinkedTransactionID,
getMostRecentReportActionLastModified,
getReportPreviewAction,
isCreatedTaskReportAction,
getParentReportAction,
Expand Down
38 changes: 34 additions & 4 deletions src/libs/SessionUtils.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
import Onyx from 'react-native-onyx';
import _ from 'underscore';
import lodashGet from 'lodash/get';
import ONYXKEYS from '../ONYXKEYS';

/**
* Determine if the transitioning user is logging in as a new user.
Expand Down Expand Up @@ -28,7 +31,34 @@ function isLoggingInAsNewUser(transitionURL, sessionEmail) {
return linkedEmail !== sessionEmail;
}

export {
// eslint-disable-next-line import/prefer-default-export
isLoggingInAsNewUser,
};
let loggedInDuringSession;

// To tell if the user logged in during this session we will check the value of session.authToken once when the app's JS inits. When the user logs out
// we can reset this flag so that it can be updated again.
Onyx.connect({
key: ONYXKEYS.SESSION,
callback: (session) => {
if (!_.isUndefined(loggedInDuringSession)) {
return;
}

if (session && session.authToken) {
loggedInDuringSession = false;
} else {
loggedInDuringSession = true;
}
},
});

function resetDidUserLogInDuringSession() {
loggedInDuringSession = undefined;
}

/**
* @returns {boolean}
*/
function didUserLogInDuringSession() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at this again, would it make more sense / be better to have some other system, like "isOnyxDataEmpty"? Maybe not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a good question since all sorts of data could be "empty" and any could be used to signify that we have not yet called OpenApp yet. On the one hand, it's a bit arbitrary, but on the other hand it is easy to reason about the "when" and "why" we would call OpenApp (post sign in) over ReconnectApp (all other times).

Looking whether Onyx data is entirely empty or not would present a new challenge. This gets pretty complicated, but the gist of it is that Onyx can initialize with various default states and when cleared we will reapply those default states. So, there really is no way for Onyx data to ever truly be "empty" as far as the App code is concerned.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation, makes sense 👍

return Boolean(loggedInDuringSession);
}

export {isLoggingInAsNewUser, didUserLogInDuringSession, resetDidUserLogInDuringSession};
102 changes: 39 additions & 63 deletions src/libs/actions/App.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import ROUTES from '../../ROUTES';
import * as SessionUtils from '../SessionUtils';
import getCurrentUrl from '../Navigation/currentUrl';
import * as Session from './Session';
import * as ReportActionsUtils from '../ReportActionsUtils';

let currentUserAccountID;
Onyx.connect({
Expand Down Expand Up @@ -43,13 +44,6 @@ Onyx.connect({
},
});

let allPolicies = [];
Onyx.connect({
key: ONYXKEYS.COLLECTION.POLICY,
waitForCollectionCallback: true,
callback: (policies) => (allPolicies = policies),
});

let preferredLocale;
Onyx.connect({
key: ONYXKEYS.NVP_PREFERRED_LOCALE,
Expand Down Expand Up @@ -137,42 +131,50 @@ AppState.addEventListener('change', (nextAppState) => {

/**
* Fetches data needed for app initialization
* @param {boolean} [isReconnecting]
*/
function openApp() {
function openApp(isReconnecting = false) {
isReadyToOpenApp.then(() => {
// We need a fresh connection/callback here to make sure that the list of policyIDs that is sent to OpenApp is the most updated list from Onyx
const connectionID = Onyx.connect({
key: ONYXKEYS.COLLECTION.POLICY,
waitForCollectionCallback: true,
callback: (policies) => {
// When the app reconnects we do a fast "sync" of the LHN and only return chats that have new messages. We achieve this by sending the most recent reportActionID.
// we have locally. And then only update the user about chats with messages that have occurred after that reportActionID.
//
// - Look through the local report actions and reports to find the most recently modified report action or report.
// - We send this to the server so that it can compute which chats are critical for the user to see and return only those as an optimization.
marcaaron marked this conversation as resolved.
Show resolved Hide resolved
const params = {policyIDList: getNonOptimisticPolicyIDs(policies)};
if (isReconnecting) {
params.mostRecentReportActionLastModified = ReportActionsUtils.getMostRecentReportActionLastModified();
}
Onyx.disconnect(connectionID);
API.read(
'OpenApp',
{policyIDList: getNonOptimisticPolicyIDs(policies)},
{
optimisticData: [
{
onyxMethod: Onyx.METHOD.MERGE,
key: ONYXKEYS.IS_LOADING_REPORT_DATA,
value: true,
},
],
successData: [
{
onyxMethod: Onyx.METHOD.MERGE,
key: ONYXKEYS.IS_LOADING_REPORT_DATA,
value: false,
},
],
failureData: [
{
onyxMethod: Onyx.METHOD.MERGE,
key: ONYXKEYS.IS_LOADING_REPORT_DATA,
value: false,
},
],
},
);

// eslint-disable-next-line rulesdir/no-multiple-api-calls
const apiMethod = isReconnecting ? API.write : API.read;
apiMethod(isReconnecting ? 'ReconnectApp' : 'OpenApp', params, {
optimisticData: [
{
onyxMethod: Onyx.METHOD.MERGE,
key: ONYXKEYS.IS_LOADING_REPORT_DATA,
value: true,
},
],
successData: [
{
onyxMethod: Onyx.METHOD.MERGE,
key: ONYXKEYS.IS_LOADING_REPORT_DATA,
value: false,
},
],
failureData: [
{
onyxMethod: Onyx.METHOD.MERGE,
key: ONYXKEYS.IS_LOADING_REPORT_DATA,
value: false,
},
],
});
},
});
});
Expand All @@ -182,33 +184,7 @@ function openApp() {
* Refreshes data when the app reconnects
*/
function reconnectApp() {
API.write(
CONST.NETWORK.COMMAND.RECONNECT_APP,
{policyIDList: getNonOptimisticPolicyIDs(allPolicies)},
{
optimisticData: [
{
onyxMethod: Onyx.METHOD.MERGE,
key: ONYXKEYS.IS_LOADING_REPORT_DATA,
value: true,
},
],
successData: [
{
onyxMethod: Onyx.METHOD.MERGE,
key: ONYXKEYS.IS_LOADING_REPORT_DATA,
value: false,
},
],
failureData: [
{
onyxMethod: Onyx.METHOD.MERGE,
key: ONYXKEYS.IS_LOADING_REPORT_DATA,
value: false,
},
],
},
);
openApp(true);
}

/**
Expand Down
9 changes: 1 addition & 8 deletions src/libs/actions/PersistedRequests.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
import Onyx from 'react-native-onyx';
import _ from 'underscore';
import CONST from '../../CONST';
import ONYXKEYS from '../../ONYXKEYS';
import HttpUtils from '../HttpUtils';

let persistedRequests = [];

Expand All @@ -19,12 +17,7 @@ function clear() {
* @param {Array} requestsToPersist
*/
function save(requestsToPersist) {
HttpUtils.cancelPendingReconnectAppRequest();
persistedRequests = _.chain(persistedRequests)
.concat(requestsToPersist)
.partition((request) => request.command !== CONST.NETWORK.COMMAND.RECONNECT_APP)
.flatten()
.value();
persistedRequests = persistedRequests.concat(requestsToPersist);
Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, persistedRequests);
}

Expand Down
2 changes: 2 additions & 0 deletions src/libs/actions/SignInRedirect.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import navigationRef from '../Navigation/navigationRef';
import SCREENS from '../../SCREENS';
import Navigation from '../Navigation/Navigation';
import * as ErrorUtils from '../ErrorUtils';
import * as SessionUtils from '../SessionUtils';

let currentIsOffline;
let currentShouldForceOffline;
Expand Down Expand Up @@ -87,6 +88,7 @@ function redirectToSignIn(errorMessage) {
NetworkConnection.clearReconnectionCallbacks();
clearStorageAndRedirect(errorMessage);
resetHomeRouteParams();
SessionUtils.resetDidUserLogInDuringSession();
}

export default redirectToSignIn;
Loading