Only fetch report history we need on reconnect + fix sign out issue #2616

marcaaron · 2021-04-28T22:28:41Z

Details

This PR is a little all over the place since I cleaned a few things up as I went, but mostly addresses two major issues:

When reconnection callbacks fire we are re-downloading report history even if we have that history stored to disk. This is inefficient and also causes a network request to fire for each chat you have - which can be quite a lot of requests and really bog things down, trigger storage eviction, etc.
I also noticed we were delaying a few network requests with timers and not cleaning them up on sign out which means that we would call them needlessly. Rather than allow this to happen I created a way to prevent that from happening in the Timers namespace. We register a timer id and then just clear them all on sign out.

Other minor issues also being cleaned up here:

NetworkConnection.stopListeningForReconnect() was being called twice for no clear reason on sign out
We were also still checking to see if we are on the signin route when signing out but that route was recently removed

Fixed Issues

Fixes #2615

Tests

Log in as USER A (easy to inspect storage and networking on web or desktop so use one of those)
Close the tab
Add several comments to a report shared with USER A as USER B (use separate session e.g. incognito tab)
In USER A browser session create a new empty tab and open Chrome Dev Tools to reveal the Network tab
Paste the sites's url and hit enter
Watch the Network tab and verify that you don't see tons of requests to Report_GetHistory and only see the one for the report with new content
Inspect the response and verify that the history array only has as many comments as were added on the previous report.
Refresh the page and wait watching the JS console for the log

[info] [Report] Local reportActions up to date. Not fetching additional actions. - {}

Log out and log back in several times and verify there are no issues doing so

QA Steps

Log in and log out repeatedly on the various platforms and verify there are no issues doing so

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

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

marcaaron · 2021-04-30T02:54:17Z

src/libs/actions/PersonalDetails.js

@@ -61,7 +61,7 @@ function getDisplayName(login, personalDetail) {
    // If we have a number like [email protected] 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);


minor fix here to get rid of a propTypes warning

marcaaron · 2021-04-30T03:13:12Z

src/libs/actions/SignInRedirect.js

-    // If we are already on the signin page, don't redirect
-    if (currentURL.indexOf('signin') !== -1) {
-        return;
-    }


This change here no longer made sense since there is no signin route

marcaaron · 2021-04-30T03:14:18Z

src/libs/actions/SignInRedirect.js

@@ -26,7 +25,6 @@ Onyx.connect({
 * @param {String} [errorMessage] error message to be displayed on the sign in page
 */
 function redirectToSignIn(errorMessage) {
-    NetworkConnection.stopListeningForReconnect();


There is a duplicate call here and this should only need to trigger once when the AuthScreens unmount

https://github.com/Expensify/Expensify.cash/blob/2ae23e75b9c7b9166fb2a333bfcdd10646508448/src/libs/Navigation/AppNavigator/AuthScreens.js#L150

marcaaron · 2021-04-30T03:46:48Z

@tgolen I'd like to get your review, but want to give @jasperhuangg and @Beamanator a shot at reviewing first if that's OK.

I'm open to feedback on any alternative ways to tackle these problems if we can identify some together. Thanks!

jasperhuangg

Tested and it works great, plus the URL issue was fixed! Thanks for the opportunity to review, it was very insightful. Feel free to add #2620 to the OP as another issue that's fixed!

src/libs/actions/Report.js

Beamanator

Looks pretty slick! Nice work! Tests also worked out well 💪

Added only a few NAB comments

src/libs/Timers.js

src/libs/CollectionUtils.js

src/libs/actions/Report.js

marcaaron · 2021-04-30T17:36:15Z

Updated

tgolen

I'm a bit confused how the two datasets play with each other, but I like the general idea!

src/CONST.js

src/libs/actions/Report.js

tgolen · 2021-04-30T17:46:10Z

src/libs/actions/Report.js

+ * 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 = {};


This is definitely confusing with maxSequenceNumber now and unfortunately, the comment doesn't clear it up enough. I think they are both listening to different keys, but I can't figure out why there would be a difference between the two, why that difference is OK, and what that difference has to do with history being available to download from the server. I think the comment can do a better job of explaining why each map is different, as well as explaining why they can get out of sync, why it's OK that they get out of sync, and what is useful about that information as it pertains to downloading new content from the server. If you want me to take a stab at it, let me know, but I think I need to make sure I get my head wrapped around this logic first before I can be of any more help.

Lemme try a bit harder here. It sounds like you're getting it, but the comment doesn't explain some things that make sense to me because I wrote this code.

I think part of the confusing is just a byproduct of having two sources to tell us about the most recent sequence number.

There might be a more holistic way to solve this. But for now, I wonder if an acceptable solution would be to:

Add a very verbose comment about what the difference between these two maps are and some background on where things are stored and why.

Take a page out of React's book and rename this map to something like UNSAFE_reportActionsMaxSequenceNumbers so that people think twice before using it.

Just went for this! I think it works and resolves a concern I have about people getting confused about which map to refer to in the future.

src/libs/actions/Report.js

tgolen · 2021-04-30T17:51:01Z

src/libs/actions/Report.js

+
+                    // 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.


Maybe this is a more helpful context to put up by the Onyx connections.

src/libs/actions/Report.js

tgolen · 2021-04-30T17:53:22Z

src/libs/actions/Report.js

-            // 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);
+            // Register the timer so we can clean it up on sign out if it has not yet fired.


How is it even possible that this timer would never fire?

Will add a comment but the case is just logging in then logging out before the timer goes.

marcaaron · 2021-04-30T22:01:12Z

Ok so after chatting with @tgolen for a while about this I eventually landed on...

Moving the maxReportActionsSequenceNumber map into a new actions/ReportActions.js file.

The intent here is that eventually other "reportActions" related actions can be moved into that file, but it would take too long to unwind right now.

The biggest concern I have with adding this new map is that it's confusing and people should find very few reasons to use it. To encourage this, I made a helper method that is exported by ReportActions.js called dangerouslyGetReportActionsMaxSequenceNumber() and it will throw angry error logs in the console unless explicitly silenced. I think that name + the console.error should discourage people from mistaking this for a more useful function.

marcaaron · 2021-04-30T22:07:43Z

Updated

Beamanator

I love the new structure and the recommendation not to use the new function you created (if possible)!

I have only a few suggestions to make some comments clearer, let me know if you disagree with any of them 👍

src/libs/actions/ReportActions.js

src/libs/actions/Report.js

src/libs/actions/ReportActions.js

tgolen

I like the new changes. It makes the situation much more clear what's happening. Thank you!

tgolen · 2021-05-03T17:26:47Z

src/libs/actions/Report.js

+
+            // 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);


Just a small thing, but the other code that users Timers.register() is passing an anonymous function, and I actually like that pattern better because it forces me to know to use Timers.register(). With this down here below the definition of the setTimeout I think it is easy to miss, so I vote for the consistency of the other pattern.

marcaaron · 2021-05-03T18:47:15Z

Updated

Beamanator

Noice 💪 #LGTM

OSBotify · 2021-05-03T22:07:06Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2021-05-04T22:38:52Z

🚀 Deployed to staging in version: 1.0.36-1🚀

platform	result
🤖 android 🤖	failure ❌
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2021-05-08T01:43:19Z

🚀 Deployed to production in version: 1.0.39-5🚀

platform	result
🤖 android 🤖	failure ❌
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

kidroca · 2021-05-08T07:10:48Z

src/libs/Timers.js

+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);
+    });
+}


const timers = []; is never emptied

clearAll will be foreach-ing already cleared timers

Maybe it can be refactored to something like

const timers = {}; function clearSingle(id) { clearTimeout(id); clearInterval(id); delete timers[id]; } function clearAll() { _.each(timers, clearSingle); }

That's a good point. But there is no danger in clearing already cleared timers so this might never be a problem.

kidroca · 2021-05-08T07:33:38Z

src/libs/actions/Report.js

-                .then((createdReportID) => {
-                    reportIDs = [createdReportID];
-                });
+            return fetchOrCreateChatReport([currentUserEmail, '[email protected]'], false);


Instead of modifying fetchOrCreateChatReport you could just return the array like:

return fetchOrCreateChatReport([currentUserEmail, '[email protected]'], false) .then(reportId => [reportId]);

IMO it's better than having to justify something fishy with a comment

// 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];

this just discloses a coupling to the fetchAllReports usage

Thanks for your notes here. I see no major problem with doing it either way.

marcaaron self-assigned this Apr 28, 2021

Use offset when fetching actions on reconnect

99f025d

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

marcaaron force-pushed the marcaaron-optimizeGetHistory branch from efc7f00 to 99f025d Compare April 29, 2021 23:58

Merge remote-tracking branch 'origin' into marcaaron-optimizeGetHistory

f974433

marcaaron changed the title ~~[WIP] Only fetch the report history that we need on reconnect~~ Only fetch report history we need on reconnect Apr 30, 2021

marcaaron changed the title ~~Only fetch report history we need on reconnect~~ Only fetch report history we need on reconnect + fix sign out issue Apr 30, 2021

marcaaron added 3 commits April 29, 2021 17:10

add clarifying comments

20735f6

Fix up diffs

c6f5cba

move timer clear to redirectToSignin

fee8795

marcaaron commented Apr 30, 2021

View reviewed changes

marcaaron marked this pull request as ready for review April 30, 2021 03:31

marcaaron requested a review from a team as a code owner April 30, 2021 03:31

MelvinBot requested review from Beamanator and removed request for a team April 30, 2021 03:31

marcaaron requested a review from jasperhuangg April 30, 2021 03:31

add more comments

1df9d3e

marcaaron requested a review from tgolen April 30, 2021 03:43

jasperhuangg previously approved these changes Apr 30, 2021

View reviewed changes

src/libs/actions/Report.js Show resolved Hide resolved

src/libs/actions/Report.js Outdated Show resolved Hide resolved

src/libs/actions/Report.js Outdated Show resolved Hide resolved

Beamanator reviewed Apr 30, 2021

View reviewed changes

src/libs/Timers.js Outdated Show resolved Hide resolved

src/libs/CollectionUtils.js Show resolved Hide resolved

src/libs/actions/Report.js Outdated Show resolved Hide resolved

src/libs/actions/Report.js Show resolved Hide resolved

Improve setting most recent action sequence number. Add comments.

09e2a9e

marcaaron dismissed jasperhuangg’s stale review via 09e2a9e April 30, 2021 17:32

tgolen requested changes Apr 30, 2021

View reviewed changes

marcaaron added 4 commits April 30, 2021 09:03

Add more verbose comments

bc4a772

Move the map to its own file and discourage use

65e8be4

make warning scarier

2dd4ccd

add logs back as debugs

9c20efd

move log

4f3680e

Split error onto several lines

9f2e572

marcaaron requested review from tgolen, Beamanator and jasperhuangg April 30, 2021 22:07

jasperhuangg previously approved these changes May 3, 2021

View reviewed changes

Beamanator requested changes May 3, 2021

View reviewed changes

src/libs/actions/ReportActions.js Outdated Show resolved Hide resolved

src/libs/actions/Report.js Outdated Show resolved Hide resolved

src/libs/actions/Report.js Outdated Show resolved Hide resolved

src/libs/actions/ReportActions.js Outdated Show resolved Hide resolved

tgolen requested changes May 3, 2021

View reviewed changes

Make requested changes

135ab15

marcaaron dismissed jasperhuangg’s stale review via 135ab15 May 3, 2021 18:20

marcaaron requested review from tgolen and Beamanator May 3, 2021 18:47

tgolen approved these changes May 3, 2021

View reviewed changes

Beamanator approved these changes May 3, 2021

View reviewed changes

Beamanator merged commit fed6fa9 into main May 3, 2021

Beamanator deleted the marcaaron-optimizeGetHistory branch May 3, 2021 22:06

marcaaron mentioned this pull request May 5, 2021

Send button has a delay of 3-4 seconds after reopening the app from background #2617

Closed

jasperhuangg mentioned this pull request May 6, 2021

Signing out yields strange SignIn Screen URL Format #2620

Closed

kidroca reviewed May 8, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only fetch report history we need on reconnect + fix sign out issue #2616

Only fetch report history we need on reconnect + fix sign out issue #2616

marcaaron commented Apr 28, 2021 •

edited

Loading

marcaaron Apr 30, 2021

marcaaron Apr 30, 2021

marcaaron Apr 30, 2021

marcaaron commented Apr 30, 2021

jasperhuangg left a comment •

edited

Loading

Beamanator left a comment

marcaaron commented Apr 30, 2021

tgolen left a comment

tgolen Apr 30, 2021

marcaaron Apr 30, 2021

marcaaron Apr 30, 2021

marcaaron Apr 30, 2021

tgolen Apr 30, 2021

tgolen Apr 30, 2021

marcaaron Apr 30, 2021

marcaaron commented Apr 30, 2021

marcaaron commented Apr 30, 2021

Beamanator left a comment

tgolen left a comment

tgolen May 3, 2021

marcaaron commented May 3, 2021

Beamanator left a comment •

edited

Loading

OSBotify commented May 3, 2021

OSBotify commented May 4, 2021

OSBotify commented May 8, 2021

kidroca May 8, 2021

marcaaron May 10, 2021

kidroca May 8, 2021

marcaaron May 10, 2021

Only fetch report history we need on reconnect + fix sign out issue #2616

Only fetch report history we need on reconnect + fix sign out issue #2616

Conversation

marcaaron commented Apr 28, 2021 • edited Loading

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcaaron commented Apr 30, 2021

jasperhuangg left a comment • edited Loading

Choose a reason for hiding this comment

Beamanator left a comment

Choose a reason for hiding this comment

marcaaron commented Apr 30, 2021

tgolen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcaaron commented Apr 30, 2021

marcaaron commented Apr 30, 2021

Beamanator left a comment

Choose a reason for hiding this comment

tgolen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcaaron commented May 3, 2021

Beamanator left a comment • edited Loading

Choose a reason for hiding this comment

OSBotify commented May 3, 2021

OSBotify commented May 4, 2021

OSBotify commented May 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcaaron commented Apr 28, 2021 •

edited

Loading

jasperhuangg left a comment •

edited

Loading

Beamanator left a comment •

edited

Loading