Fix ActiveClientManager doesn't track the currently active tab well and needs improved #40997

Skakruk · 2024-04-25T16:20:44Z

Details

Fixed Issues

$ #40044
PROPOSAL: #40044 (comment)

Tests

The easiest way to track if app client is a leader is to check the login page.

Client receives leading status:

Open the login page as an unauthenticated user in Tab A
Right-click on the tab and select Duplicate
Switch to Tab A before Tab B is fully loaded
Check that Another login page is open. text appears instead of the login form
Right-click on Tab B while it's in the background and select Close
Check that Another login page is open. disappears and the normal login form is shown.

Client correctly receives leading status on tab change:

Open the login page as an unauthenticated user in Tab A
Right-click on the tab and select Duplicate
Wait until app is fully loaded
The login form is displayed
Switch to Tab A
The login form is displayed

Offline tests

Not applicable.

QA Steps

The easiest way to track if app client is a leader is to check the login page.

Client receives leading status:

Open the login page as an unauthenticated user in Tab A
Right-click on the tab and select Duplicate
Switch to Tab A before Tab B is fully loaded
Check that Another login page is open. text appears instead of the login form
Right-click on Tab B while it's in the background and select Close
Check that Another login page is open. disappears and the normal login form is shown.

Client correctly receives leading status on tab change:

Open the login page as an unauthenticated user in Tab A
Right-click on the tab and select Duplicate
Wait until app is fully loaded
The login form is displayed
Switch to Tab A
The login form is displayed

PR Author Checklist

Screenshots/Videos

Android: Native

Not applicable

Android: mWeb Chrome

Not applicable

iOS: Native

Not applicable

iOS: mWeb Safari

Not applicable

MacOS: Chrome / Safari

Screen.Recording.2024-04-25.at.19.15.32.mp4

MacOS: Desktop

Not applicable

melvin-bot · 2024-04-25T16:20:49Z

@DylanDylann Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

github-actions · 2024-04-25T16:20:55Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

Skakruk · 2024-04-25T16:22:38Z

I have read the CLA Document and I hereby sign the CLA

DylanDylann · 2024-04-26T05:17:59Z

@allroundexperts Will you take over this one?

allroundexperts · 2024-04-26T09:05:10Z

Yes. Not sure why I wasn't assigned here. @tgolen can you assign this to me? Thanks!

@Skakruk Can you please complete the CLA?

Skakruk · 2024-04-26T09:09:45Z

@allroundexperts I did, bot has changed the comment text, but the job is still in a failed state. Not sure how to fix it. Tried the recheck command too a few times.

tgolen · 2024-04-26T14:15:52Z

OK, looks like I got the CLA test to pass. You'll need to run prettier on the branch to clear up the ESLint test that is failing.

Skakruk · 2024-04-26T14:27:25Z

Fixed linting

allroundexperts · 2024-04-28T21:01:54Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android: Native
- Android: mWeb Chrome
- iOS: Native
- iOS: mWeb Safari
- MacOS: Chrome / Safari
- MacOS: Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
- I verified that all the inputs inside a form are aligned with each other.
- I added Design label and/or tagged @Expensify/design so the design team can review the changes.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: Native

Android: mWeb Chrome

Screen.Recording.2024-04-30.at.2.07.24.AM.mov

iOS: Native

iOS: mWeb Safari

Screen.Recording.2024-04-30.at.2.05.13.AM.mov

MacOS: Chrome / Safari

Screen.Recording.2024-04-30.at.1.59.11.AM.mov

MacOS: Desktop

allroundexperts · 2024-04-29T21:13:07Z

Hi @Skakruk!
The solution you applied here is quite different from what you proposed ie:

window.addEventListener("beforeunload",  () => {
        activeClients = activeClients.filter((id) => id !== clientID);
        ActiveClients.setActiveClients(activeClients);
    });

Can you please explain why the above was not sufficient?

allroundexperts · 2024-04-29T21:14:39Z

Also, using storage event would degrade the performance of the app for sure. Can't we just stick to your original proposal?

Skakruk · 2024-04-30T06:46:42Z

@allroundexperts sure, here is my thoughts:

First, I implemented as I described in the proposal, and everything was working. But I dug deeper in docs and found out that using event beforeunload is not recommended. It makes page ineligible for bfcache and it should be used only conditionally (e.g. when we need to notify a user of an unfinished job on the form). https://web.dev/articles/bfcache#beforeunload-caution

The recommended events are visibilitychange and pagehide. The first one is called on every time the tab goes background and the second is when the tab is closed. So I went with the second one.

But since those event callbacks are sync, it's not guaranteed that that transaction to IDB will finish in time (in almost all tries it didn't). Onyx.set and Onyx.subscription worked fine, but no change to persistent storage.

So I had to manage it is some way, that the closing tab will communicate with the left ones and notify them of client being destroyed in sync way.

There are a couple of ways to archive it:

Broadcast Channel API - which is a fairly new API, but already implemented in all browsers https://caniuse.com/broadcastchannel
Using localStorage and storage event - old-fashioned style of communication between tabs.
Use https://www.npmjs.com/package/broadcast-channel - which utilizes both of them, depending on browser support.

I went with localStorage option since it's working in all browsers.

I can go with Broadcast Channel API, but I couldn't find any drawbacks of using storage event (maybe I'm missing some).

allroundexperts · 2024-04-30T08:34:18Z

Hi @Skakruk!
In my opinion, we should use onunload since it is much more simpler and less complex. The complexity which gets added due to the storage listener is not worth it. @tgolen Can you please weigh in here?

Skakruk · 2024-04-30T08:46:04Z

@allroundexperts onunload event is deprecated and is not recommended for use https://developer.mozilla.org/en-US/docs/Web/API/Window/unload_event https://developer.chrome.com/docs/web-platform/deprecating-unload

allroundexperts · 2024-05-05T21:09:03Z

@Skakruk I do get that. But I'm not fully onboard with the solution you implemented. I feel like its too complex. Waiting to see what @tgolen thinks.

tgolen · 2024-05-06T18:55:42Z

I think I am following most of your solution here. I understand most of the thought process that you went through, but I'm a little unsure about this:

But since those event callbacks are sync, it's not guaranteed that that transaction to IDB will finish in time (in almost all tries it didn't). Onyx.set and Onyx.subscription worked fine, but no change to persistent storage.

Onyx is already firing a storage event for any data changes here: https://github.com/Expensify/react-native-onyx/blob/main/lib/storage/InstanceSync/index.web.ts#L18

Are you saying that this code never executes before the visibilitychange and pagehide events have ran their callbacks?

First, I implemented as I described in the proposal, and everything was working.

I kind of think that we should just go back to this for now. Until there is a specific problem with that approach, let's just stick with it since it is more simple. Once those events are fully deprecated, then we can come back to this and change it if we need to.

Skakruk · 2024-05-07T10:43:47Z

@tgolen

Are you saying that this code never executes before the visibilitychange and pagehide events have ran their callbacks?

Onyx does its job correctly, but it does not save changes to IndexedDB, since those calls are async.

I kind of think that we should just go back to this for now. Until there is a specific problem with that approach

I'm afraid that using beforeunload event might affect the overall performance of the application as it prevents usage of bfcache.

tgolen · 2024-05-07T15:18:40Z

might affect the overall performance of the application as it prevents usage of bfcache.

Have you confirmed this is an actual problem?

Skakruk · 2024-05-08T18:51:02Z

@tgolen I rechecked and could not observe the issues described in article about bfcache.

So I reverting the implementation to the basic one.

tgolen

Wonderful, thanks for confirming that! This looks good.

allroundexperts

Looks good to me as well!

OSBotify · 2024-05-10T14:34:49Z

✋ 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 · 2024-05-13T08:42:09Z

🚀 Deployed to staging by https://github.com/tgolen in version: 1.4.73-0 🚀

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

kbecciv · 2024-05-13T17:01:28Z

This PR is failing because of issue #40044

The issue is reproducible in: Web - only check for now

1715596548809.40997_web.mp4

OSBotify · 2024-05-15T09:50:19Z

🚀 Deployed to production by https://github.com/Beamanator in version: 1.4.73-7 🚀

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

Add tracking of closed clients

2d472a6

Skakruk requested a review from a team as a code owner April 25, 2024 16:20

melvin-bot bot requested review from DylanDylann and removed request for a team April 25, 2024 16:20

Skakruk mentioned this pull request Apr 25, 2024

[HOLD for payment 2024-05-22] [$250] [Bug] ActiveClientManager doesn't track the currently active tab well and needs improved #40044

Closed

tgolen requested a review from allroundexperts April 26, 2024 14:05

Fix linting

93793e6

Use beforeunload event only and simplify logic

0de4ae2

tgolen approved these changes May 8, 2024

View reviewed changes

allroundexperts approved these changes May 9, 2024

View reviewed changes

melvin-bot bot requested a review from tgolen May 9, 2024 18:33

tgolen merged commit d7216ea into Expensify:main May 10, 2024
15 of 16 checks passed

github-actions bot mentioned this pull request May 13, 2024

Deploy Checklist: New Expensify 2024-05-13 #42040

Closed

74 tasks

amyevans mentioned this pull request May 13, 2024

[HOLD for payment 2024-05-22] Login - 'Another login page is open' briefly displayed when refresh the page #42062

Closed

6 tasks

Skakruk mentioned this pull request May 13, 2024

[CP Staging] Keep original isClientTheLeader value on page refresh #42095

Merged

54 tasks

Skakruk deleted the 40044-track-active-tab branch May 13, 2024 18:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ActiveClientManager doesn't track the currently active tab well and needs improved #40997

Fix ActiveClientManager doesn't track the currently active tab well and needs improved #40997

Skakruk commented Apr 25, 2024 •

edited

Loading

melvin-bot bot commented Apr 25, 2024

github-actions bot commented Apr 25, 2024 •

edited

Loading

Skakruk commented Apr 25, 2024

DylanDylann commented Apr 26, 2024

allroundexperts commented Apr 26, 2024

Skakruk commented Apr 26, 2024

tgolen commented Apr 26, 2024

Skakruk commented Apr 26, 2024

allroundexperts commented Apr 28, 2024 •

edited

Loading

allroundexperts commented Apr 29, 2024

allroundexperts commented Apr 29, 2024

Skakruk commented Apr 30, 2024

allroundexperts commented Apr 30, 2024

Skakruk commented Apr 30, 2024

allroundexperts commented May 5, 2024

tgolen commented May 6, 2024

Skakruk commented May 7, 2024

tgolen commented May 7, 2024

Skakruk commented May 8, 2024

tgolen left a comment

allroundexperts left a comment

OSBotify commented May 10, 2024

OSBotify commented May 13, 2024

kbecciv commented May 13, 2024 •

edited

Loading

OSBotify commented May 15, 2024

Fix ActiveClientManager doesn't track the currently active tab well and needs improved #40997

Fix ActiveClientManager doesn't track the currently active tab well and needs improved #40997

Conversation

Skakruk commented Apr 25, 2024 • edited Loading

Details

Fixed Issues

Tests

Client receives leading status:

Client correctly receives leading status on tab change:

Offline tests

QA Steps

Client receives leading status:

Client correctly receives leading status on tab change:

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Apr 25, 2024

github-actions bot commented Apr 25, 2024 • edited Loading

Skakruk commented Apr 25, 2024

DylanDylann commented Apr 26, 2024

allroundexperts commented Apr 26, 2024

Skakruk commented Apr 26, 2024

tgolen commented Apr 26, 2024

Skakruk commented Apr 26, 2024

allroundexperts commented Apr 28, 2024 • edited Loading

Reviewer Checklist

Screenshots/Videos

allroundexperts commented Apr 29, 2024

allroundexperts commented Apr 29, 2024

Skakruk commented Apr 30, 2024

allroundexperts commented Apr 30, 2024

Skakruk commented Apr 30, 2024

allroundexperts commented May 5, 2024

tgolen commented May 6, 2024

Skakruk commented May 7, 2024

tgolen commented May 7, 2024

Skakruk commented May 8, 2024

tgolen left a comment

Choose a reason for hiding this comment

allroundexperts left a comment

Choose a reason for hiding this comment

OSBotify commented May 10, 2024

OSBotify commented May 13, 2024

kbecciv commented May 13, 2024 • edited Loading

OSBotify commented May 15, 2024

Skakruk commented Apr 25, 2024 •

edited

Loading

github-actions bot commented Apr 25, 2024 •

edited

Loading

allroundexperts commented Apr 28, 2024 •

edited

Loading

kbecciv commented May 13, 2024 •

edited

Loading