Enable active client manager, multiple tabs, and previous PR cleanup #469

tgolen · 2020-09-11T15:48:19Z

Cleans up some of the final comments that were in Refactor the rest of the code to remove Ion.get() #426
Enables the active client manager and multi-tab functionality to remove some TODOs in the code Fixes https://github.com/Expensify/Expensify/issues/141684
Adds a warning for some undefined behavior with Ion.connect() and provides instructions on proper use

Tests for multi-tab functionality

Open multiple tabs to the chat application with the same USER A signed in
Inspect localStorage to see that activeClients has multiple GUIDs
In a different browser (or in an incognito window), sign in to the chat application as USER B (make sure both users have access to the same reports)
As USER B, send a comment to any chat that USER A is not currently looking at
Verify there is a browser notification and clicking it should take you to one of the chat apps for USER A (the first tab you opened)

Go to the second tab for USER A and check the JS console (you need to have verbose logging turned on) and verify you see: [NOTIFICATION] Skipping notification because this client is not the leader
Now close the first tab for USER A
Go back to USER B and add a new comment
Verify there is a browser notification and clicking it should take you to the tab for USER A that is still open

Fixes #467

cead22 · 2020-09-11T20:02:59Z

src/lib/Ion.js

@@ -1,39 +1,13 @@
 import _ from 'underscore';
 import AsyncStorage from '@react-native-community/async-storage';
+import StorageEvent from './StorageEvent';


NAB but importing this as addStorageEventListener would make init read like this and imo be clearer

function init() { addStorageEventListener((key, newValue) => keyChanged(key, newValue)); }

cead22 · 2020-09-11T20:42:55Z

src/lib/Ion.js

-    // Values that are objects can be merged into storage
-    if (_.isObject(val)) {
+    // Values that are objects or arrays are merged into storage
+    if (_.isObject(val) || _.isArray(val)) {


Second condition is redundant, but the comment is good

> _.isObject([]) > true

Returns true if value is an Object. Note that JavaScript arrays and functions are objects, while (normal) strings and numbers are not.

Ah, you know, I had read that... but I ran into issues with that and I needed to add this code. Let me try again.

tgolen · 2020-09-14T17:50:22Z

Updated with both suggestions 👍

marcaaron

Nice! Looks great to me. Just have a few NAB/questions.

marcaaron · 2020-09-14T17:45:37Z

src/Expensify.js

-            </Router>
-
-        // </Beforeunload>
+            <Beforeunload onBeforeunload={ActiveClientManager.removeClient}>


weird that this is not onBeforeUnload is it something we control?

That's straight from the lib. Now that I think about it though, we could definitely just write this ourselves and not need a third-party lib to do it.

marcaaron · 2020-09-14T17:48:01Z

src/Expensify.js

+                        <Route path={['/home', '/']} component={HomePage} />
+                    </Switch>
+                </Router>
+            </Beforeunload>


I'm curious why we are using this as a wrapper and not render with a self-closing tag? Is there a reason?

No reason. In fact, if we're writing our own version, I'd rather just have it be a utility method as opposed to some component that's rendered.

Yep you are reading my mind 😅

marcaaron · 2020-09-14T17:50:29Z

src/lib/ActiveClientManager.js

-const init = () => Ion.set(IONKEYS.ACTIVE_CLIENTS, {[clientID]: clientID});
+function init() {
+    Ion.merge(IONKEYS.ACTIVE_CLIENTS, {[clientID]: clientID});
+}

 /**
 * Remove this client ID from the array of active client IDs when this client is exited


NAB, maybe use map instead of array here just to avoid confusion as the activeClients is an Object and not Array

marcaaron · 2020-09-14T17:55:34Z

src/lib/ActiveClientManager.js

-    // At the moment activeClients only has 1 value i.e., the latest clientID so let's compare if
-    // the latest matches the current browsers clientID.
-    return activeClients[clientID] === clientID;
+    return _.first(_.keys(activeClients)) === clientID;


Is there any reason we are using an object and not an array? I know if we are adding a new string key to an object that the order will be preserved. But is the same true for Ion.merge()?

I tried using an array, and I ran into a lot of problems with it being converted into a strange object instead like {0: '1234', 1: '3456'}. I can try digging into this more.

Oh that's interesting. Is this just a limitation of AsyncStorage when we try to save an array and get it again? Or does this happen when we use AsyncStorage.mergeItem()?

OK, I boiled this down to an implementation "detail" (maybe bug) of async storage:

AsyncStorage.mergeItem('timtest', JSON.stringify([1]));

So... merging doesn't really work with arrays. I am going to add a solution where we manually merge arrays inside of Ion.merge() to have the expected behavior.

This reverts commit aa4811f.

tgolen · 2020-09-14T23:53:27Z

Updated with simplifications!

One thing that I have seen happen is that it's fairly easy for an active client GUID to get stuck at the front of the array. I've seen this happen multiple times in local development, but I don't know if it was just because of errors in the code, or actual behavior. @marcaaron I've changed the name of the storage key to help with this (and not conflict with what was previously stored), but please keep an eye out for this. If it happens in production, it's bad because it would mean that web would never have notifications happening. I don't know if there is some sort of "fail-safe" that we can add to this to ensure that this situation would be minimized. Maybe like the GUID expiring after a certain amount of time? I don't know...

marcaaron · 2020-09-15T00:01:38Z

src/lib/addStorageEventHandler/index.js

+ */
+function addStorageEventHandler(callback) {
+    window.addEventListener('storage', (e) => {
+        callback(e.key, JSON.parse(e.newValue));


Just wondering if we should add any sort of check to see if the localStorage changes are actually related to our app as other things (chrome extensions etc) might possibly be sharing localStorage? Maybe it doesn't matter since we'll call keyChanged() and won't have any subscribers? But it would be better to not call that at all.

Let's not worry about that unless it becomes an issue 👍

Ok that works for me. It's easy enough to add in later if it becomes a problem since we should theoretically list all keys in IONKEYS

We can also wait to see if this is a problem, but I was thinking of wrapping this in try/catch in case newValue isn't valid JSON, but I don't know how that would happen

Ah, adding a try/catch for the json.parse() is a good idea. Let me add that.

tgolen · 2020-09-16T13:37:45Z

Would it be possible to get this merged soon?

marcaaron

LGTM just left one more comment about Ion.merge with arrays and there are conflicts.

marcaaron · 2020-09-16T16:26:57Z

src/IONKEYS.js

@@ -2,7 +2,7 @@
 * This is a file containing constants for all the top level keys in our store
 */
 export default {
-    ACTIVE_CLIENTS: 'activeClients',
+    ACTIVE_CLIENTS: 'activeClients2',


Is this intentional?

Yeah, it was intentional... and I'm open to a better solution. The problem is that these were stored as an object before, and sense we are only calling Ion.merge() for this, moving from an object to an array is not possible and will result in really bad values being stored.

One possible solution is to rename it to maybe activeClientIDs, then add some code which clears out activeClients, and then after a week, we can go back and set it back to activeClients. Whatever solution we think of here, we should try to find some kind of method that is fairly reproducible because I think we will need to do this occassionally.

Ok changing the name works for now!

I think learn term we need a way to perform localStorage migrations for when keys change by adding some kind of instructions before the app updates or inits. That might mean we have some migration code running that based on the version will alter the localStorage schema or simply blow it away and start over from scratch minus session information.

Seems like a similar problem people run into when implementing a ServiceWorker

I agree with this. The way I imagine this would work is something like

If the new version of the app renames a key or wants to clear old keys

When the app inits

Clear local storage (like signout does)

Re-authenticate

Load everything from scratch

That should all be transparent to the user

Yeah, I think the catch there is that process wouldn't be exactly "transparent" to the user, particularly the "load everything from scratch" part. I'm going to spin up an issue so we can discuss this further and plan on leaving this code the way it is for now.

marcaaron · 2020-09-16T16:29:57Z

src/lib/Ion.js

+                console.warn(`It looks like a React component subscribed to multiple Ion keys without 
+                providing an 'indexBy' option. This will result in undefined behavior. The best thing to do is 
+                provide an 'indexBy' value, or use a more specific regex that will only match a single Ion key.`);
+            }


Can maybe be removed after merging master?

src/lib/Ion.js

marcaaron · 2020-09-16T16:34:53Z

src/lib/addStorageEventHandler/index.js

+ */
+function addStorageEventHandler(callback) {
+    window.addEventListener('storage', (e) => {
+        callback(e.key, JSON.parse(e.newValue));


Ok that works for me. It's easy enough to add in later if it becomes a problem since we should theoretically list all keys in IONKEYS

tgolen · 2020-09-16T21:39:57Z

Updated, thanks!

marcaaron

Hmm wait something is not right actually. When I start up the app with localStorage completely cleared somehow I get this...

marcaaron · 2020-09-16T22:12:38Z

Oh that's interesting so... localStorage.clear() or clearing storage from Chrome dev tools does not actually clear AsyncStorage...

marcaaron · 2020-09-16T22:15:47Z

src/lib/ActiveClientManager/index.js

+ * @returns {boolean}
+ */
+function isClientTheLeader() {
+    return _.first(activeClients) === clientID;


Just double checking something here. Wouldn't it be safer to give priority to the last client that was added instead of the first?

e.g. if a beforeunload event doesn't happen then I think we'll might get a zombie "leader" that disappeared a long time ago. Which would leave us in a bad state since we are pushing new clients onto the end of the array. But this wouldn't happen if new clients were pushed onto the front of the array or we treated the last item as the "leader".

tgolen · 2020-09-16T22:19:46Z

Yeah, so I think you're seeing what I mentioned earlier about GUIDs getting stuck in that array.

clearing storage from Chrome dev tools does not actually clear AsyncStorage...

I am 99% sure that clearing local storage directly does clear AsyncStorage (which is just using local storage underneath). I think something else is happening here. First thing I always check is that there are no other windows open (because those will for sure be adding GUIDs into the array). I think I like the suggestion about using the last GUID instead of the first GUID! That makes sense and should solve that problem hopefully.

marcaaron · 2020-09-16T22:24:14Z

Yeah thinking about it some more... we don't really even need a beforeunload event. We just need a cap on how many open tabs a person might have and then set a hard limit on how many we'd support. Storing something like 20 IDs in an array probably would work.

tgolen · 2020-09-17T01:56:31Z

Hm, so when we reach the max, then we'd just start removing the oldest GUID? That could work

tgolen · 2020-09-17T02:05:02Z

Yep, that works! Updated.

tgolen · 2020-09-21T20:31:28Z

Bump for review please :)

marcaaron · 2020-09-21T20:53:19Z

@AndrewGable or @cead22 want to take a look?

cead22 · 2020-09-21T22:34:07Z

Conflicts

cead22 · 2020-09-22T01:58:28Z

Sorry, I didn't get a chance to review today, but I'll give this a look tomorrow

tgolen · 2020-09-22T13:44:41Z

Conflicts resolved.

cead22 · 2020-09-22T23:05:56Z

src/lib/addStorageEventHandler/index.js

+ */
+function addStorageEventHandler(callback) {
+    window.addEventListener('storage', (e) => {
+        callback(e.key, JSON.parse(e.newValue));


We can also wait to see if this is a problem, but I was thinking of wrapping this in try/catch in case newValue isn't valid JSON, but I don't know how that would happen

cead22 · 2020-09-22T23:10:48Z

src/IONKEYS.js

@@ -2,7 +2,7 @@
 * This is a file containing constants for all the top level keys in our store
 */
 export default {
-    ACTIVE_CLIENTS: 'activeClients',
+    ACTIVE_CLIENTS: 'activeClients2',


I agree with this. The way I imagine this would work is something like

If the new version of the app renames a key or wants to clear old keys

When the app inits

Clear local storage (like signout does)

Re-authenticate

Load everything from scratch

That should all be transparent to the user

cead22 · 2020-09-22T23:14:57Z

src/lib/ActiveClientManager/index.js

+
+    callback: (val) => {
+        activeClients = val;
+        if (activeClients.length >= maxClients) {


Don't we need to update active clients in Ion in the else case?

I don't see why we would, no. Can you explain why you're thinking it would be necessary? If you added a set() in the else, then the value of active clients in Ion wouldn't be changing at all.

I don't think I fully understood how this works yesterday, but I think I get it now. When init is called, we just add clients. If somebody calls init and we reach the max of clients, we remove the first one that was added 👍

Yep! Exactly.

tgolen · 2020-09-23T16:58:21Z

Updated with a try/catch for the JSON parsing

cead22 · 2020-09-23T16:59:47Z

src/lib/addStorageEventHandler/index.js

+        } catch (err) {
+            console.error('Could not parse the newValue of the storage event', err, e);
+        }
+        callback(e.key, JSON.parse(newValue));


This should be callback(e.key, newValue);

ah, haha 🤦

(and tested)

tgolen added 5 commits September 11, 2020 08:35

Address some simple comments from previous PR review

3d6b637

Implement multi-tab functionality

8814c20

Add a warning when there can be undefined behavior

fa99401

Fixes #467

Remove TODO

0da38df

Fix mobile

61aaa85

tgolen requested review from cead22 and marcaaron September 11, 2020 15:48

tgolen requested a review from AndrewGable as a code owner September 11, 2020 15:48

tgolen self-assigned this Sep 11, 2020

cead22 previously approved these changes Sep 11, 2020

View reviewed changes

Rename storage event export

1720bad

tgolen dismissed cead22’s stale review via 1720bad September 14, 2020 17:50

marcaaron reviewed Sep 14, 2020

View reviewed changes

marcaaron mentioned this pull request Sep 14, 2020

Duplicate desktop notifications for same chat. #474

Closed

tgolen added 7 commits September 14, 2020 17:00

Merge branch 'master' into tgolen-refactor-cleanup

1fa3262

Move active client manager to a platform specific lib

21ce6dc

Move add storage event listener into platform specific code

aa4811f

Revert "Move add storage event listener into platform specific code"

fe8fc83

This reverts commit aa4811f.

Rename method to be more clear

47d0ca1

Improve Ion.merge() to work better with arrays

cb67d7e

Simplify handling unload event

abc9cb0

Fix inconsistent native API

457f1bb

marcaaron reviewed Sep 15, 2020

View reviewed changes

marcaaron requested changes Sep 16, 2020

View reviewed changes

tgolen added 2 commits September 16, 2020 15:35

Merge branch 'master' into tgolen-refactor-cleanup

2992aa0

Protect against null values

228bc2d

marcaaron self-requested a review September 16, 2020 22:09

marcaaron requested changes Sep 16, 2020

View reviewed changes

marcaaron reviewed Sep 16, 2020

View reviewed changes

Remove onbeforeunload event

232192d

Merge branch 'master' into tgolen-refactor-cleanup

20b9be5

marcaaron previously approved these changes Sep 21, 2020

View reviewed changes

Merge branch 'master' into tgolen-refactor-cleanup

a418d90

tgolen dismissed marcaaron’s stale review via a418d90 September 22, 2020 13:44

cead22 previously approved these changes Sep 22, 2020

View reviewed changes

Catch errors when trying to parse JSON

38c19e1

tgolen dismissed cead22’s stale review via 38c19e1 September 23, 2020 16:58

cead22 requested changes Sep 23, 2020

View reviewed changes

tgolen added 2 commits September 23, 2020 11:00

Don't double-parse

3166611

Only trigger the callback when there was a valid value that was parsed

90ea5a4

cead22 approved these changes Sep 23, 2020

View reviewed changes

cead22 merged commit a858268 into master Sep 23, 2020

cead22 deleted the tgolen-refactor-cleanup branch September 23, 2020 21:51

snyk-bot mentioned this pull request May 14, 2021

[Snyk] Upgrade react-native-modal from 11.5.6 to 11.10.0 #2943

Closed

ZhenjaHorbach mentioned this pull request Sep 13, 2024

Bump react-native-live-markdown to newest version and remove/rename patches #48626

Merged

50 tasks

Enable active client manager, multiple tabs, and previous PR cleanup #469

Enable active client manager, multiple tabs, and previous PR cleanup #469

Conversation

tgolen commented Sep 11, 2020 • edited by AndrewGable Loading

Tests for multi-tab functionality

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgolen commented Sep 14, 2020

marcaaron 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgolen commented Sep 14, 2020

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

tgolen commented Sep 16, 2020

marcaaron 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

tgolen commented Sep 16, 2020

marcaaron left a comment

Choose a reason for hiding this comment

marcaaron commented Sep 16, 2020

Choose a reason for hiding this comment

tgolen commented Sep 16, 2020

marcaaron commented Sep 16, 2020

tgolen commented Sep 17, 2020

tgolen commented Sep 17, 2020

tgolen commented Sep 21, 2020

marcaaron commented Sep 21, 2020

cead22 commented Sep 21, 2020

cead22 commented Sep 22, 2020

tgolen commented Sep 22, 2020

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

tgolen commented Sep 23, 2020

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

tgolen commented Sep 11, 2020 •

edited by AndrewGable

Loading