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

[tgolen]

Problem

The purpose of ActiveClientManager is to ensure that when there are multiple tabs open, only one tab handles write requests. There are a few bugs with this.

In order to test this, I used the following diff and then ran ActiveClientManager.isClientTheLeader() from the command line.

diff --git a/src/setup/addUtilsToWindow.ts b/src/setup/addUtilsToWindow.ts
index d2d11e1384..30419a5eb5 100644
--- a/src/setup/addUtilsToWindow.ts
+++ b/src/setup/addUtilsToWindow.ts
@@ -1,6 +1,7 @@
 import Onyx from 'react-native-onyx';
 import * as Environment from '@libs/Environment/Environment';
 import markAllPolicyReportsAsRead from '@libs/markAllPolicyReportsAsRead';
+import * as ActiveClientManager from '@libs/ActiveClientManager';
 import * as Session from '@userActions/Session';

 /**
@@ -49,5 +50,7 @@ export default function addUtilsToWindow() {
         // Workaround to give employees the ability to mark reports as read via the JS console
         // eslint-disable-next-line @typescript-eslint/no-explicit-any
         (window as any).markAllPolicyReportsAsRead = markAllPolicyReportsAsRead;
+
+        window.ActiveClientManager = ActiveClientManager;
     });
 }

Bug 1: Multiple tabs becoming active

1. Open a single tab (TabA)
2. Verify isClientTheLeader() === true
3. Open a second tab (TabB) in the same window
4. Verify isClientTheLeader() === true
5. Go back to TabA

Expected result:
isClientTheLeader() === false because TabB took over as the new leader

Actual result:
isClientTheLeader() === true and both tabs think they are the leader

Bug 2: If active tab is closed, there are no active tabs

Note

I was unable to reproduce this because of Bug1. However, once Bug1 is fixed, I think this bug would happen.

1. Have TabA open and be the active client
2. Have TabB open and be a non-active client
3. Close TabA

Expected result:
Tab B takes over and becomes the new active client

Actual result:
Tab B remains as an inactive client

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~012e35885da3cd69cf
• Upwork Job ID: 1778173291530903552
• Last Price Increase: 2024-04-17

Issue Owner

Current Issue Owner: @isabelastisser

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~012e35885da3cd69cf

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @Santhosh-Sellavel (External)

[melvin-bot]

Triggered auto assignment to @isabelastisser (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[Skakruk]

Proposal

Please re-state the problem that we are trying to solve in this issue.

I believe Bug 1 is not an issue, since the client becomes the lead once the user activates the tab.
In the test, when we have 2 windows - one per account, and in first window opens 2 tabs, we write from the second account and we can spot that only one is reporting isClientTheLeader() === true.

two-tabs.mp4

Bug 2 If active tab is closed, there are no active tabs

What is the root cause of that problem?

Bug 2.

Indeed, when the second tab is closing, we still preserve its clientID in Onyx. But this happens, only when we close the lead tab after navigating to some 3rd party tab. And will not activate the following tab (after closing tab browser will not be focused on the following tab).

The root cause is that the following tab is not receiving updates on active clients. It still handles the list of active clients, which is not the least created one.

What changes do you think we should make in order to solve the problem?

I propose to add beforeunload event listener so that the client will clean up its clientID from the list of active clients just before it is closed (or reloaded).

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

In this case, any of the remaining app tabs will become a leader. And, as a bonus, the activeClients list will not grow.

What alternative solutions did you explore? (Optional)

[gitaumoses4]

Proposal

Please re-state the problem that we are trying to solve in this issue

ActiveClientManager does not track the current active tab correctly, especially when the active tab is closed.

What is the root cause of that problem?

Bug 1

This can be viewed as a bug, or a desired outcome based on what is needed.

If we want the currently active tab to be the client leader - then it works as expected
If we want the newly created tab to be the client leader - then this can be considered a bug.

The currently activated tab by the user, takes in the role of the "Active client". This means that when a user:

1. Opens a single tab (Tab A) - it takes in the role of client leader
2. Opens another tab (Tab B) - it takes in the role of the client leader
3. Switches from Tab B to tab A - Tab A takes in the role of the client leader

This means that we only have one client leader since it's activated when a user opens/reloads the tab.

This happens because the init method is invoked on the active tab to make it the client leader.

const init: Init = () => {
    activeClients = activeClients.filter((id) => id !== clientID);
    activeClients.push(clientID);
    ActiveClients.setActiveClients(activeClients).then(resolveSavedSelfPromise);
};

Bug 2

When a tab is closed, the clientID is not released from the list of active clients. This causes the other tabs remain in an inactive state.

What changes do you think we should make in order to solve the problem?

Bug 1

If we consider this a bug, then we can make the following changes.

let isInitialized = false;
const init: Init = () => {
    if( isInitialized ) return;

    activeClients = activeClients.filter((id) => id !== clientID);
    activeClients.push(clientID);
    ActiveClients.setActiveClients(activeClients).then(resolveSavedSelfPromise);
    isInitialized = true;
};

This results in the newly created/reloaded tab taking over as the active client as the init function is not invoked when the tab is re-activated.

The advantage of this is that the client leader does not keep changing as the user changes tabs, and only when a new tab is created/or an existing one is reloaded.

Bug 2

When a tab is closed, or the browser window is closed, we can detect this using the beforeunload event.

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

This will notify other tabs of the release of the currently active tab.
The previously active tab will now take over the role of client leader. (Whether it's a currently active browser tab or not)

What alternative solutions did you explore? (Optional)

N/A

[melvin-bot]

📣 @gitaumoses4! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Make sure you've read and understood the contributing guidelines.
2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[gitaumoses4]

Contributor details
Your Expensify account email: [email protected]
Upwork Profile Link: https://www.upwork.com/freelancers/gitau

[melvin-bot]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[ShridharGoel]

Proposal

Please re-state the problem that we are trying to solve in this issue.

ActiveClientManager changes the leader tab whenever focus changes + client ID is not deleted from active clients on tab close and reload.

What is the root cause of that problem?

Bug 1:

initializeClient is being called when app state changes.

App/src/Expensify.tsx

Line 181 in 260a7de

appStateChangeListener.current = AppState.addEventListener('change', initializeClient);

This leads to init() being called in ActiveClientManager which pushes the tab to the end of the list.

App/src/Expensify.tsx

Lines 117 to 123 in 260a7de

	const initializeClient = () => {
	if (!Visibility.isVisible()) {
	return;
	}
	ActiveClientManager.init();
	};

Hence, whenever we switch to any tab, init method is called again and the focused tab becomes the leader.

So, when only tab A is open, it is the leader as expected. Now, tab B is opened, then it becomes the leader as expected.
But, if we go again to tab A, then it will again become the leader because app state change calls the init method. So, whatever tab is focused will take the leader role.

Bug 2:

The list of active clients does not update when a tab is closed. This means the list even contains clientIDs which are no longer active, hence the leader does not change if the leader tab is closed.

What changes do you think we should make in order to solve the problem?

Fix for bug 1:

ActiveClientManager.init(); is being called inside useLayoutEffect which is enough. We don't need to call it on app state changes.

We can remove this call:

appStateChangeListener.current = AppState.addEventListener('change', initializeClient);

And this method also:

const initializeClient = () => {
    if (!Visibility.isVisible()) {
        return;
    }

    ActiveClientManager.init();
};

Fix for bug 2:

Add a handle tab close method:

const handleTabClose = () => {
    activeClients = activeClients.filter((id) => id !== clientID);
    ActiveClients.setActiveClients(activeClients)
        .then(() => {
            window.removeEventListener("beforeunload", handleTabClose);
            resolveSavedSelfPromise()
        })
        .catch((error) => {
            console.error("Error updating active clients:", error);
        });
};

Add an event listener for beforeunload:

window.addEventListener("beforeunload",  (e) => {
    handleTabClose()
});

This works with both tab close and page refresh (tested). Although, note that you need to click atleast once somewhere on the page before refreshing, otherwise beforeunload will be not listened.

[tgolen]

@Santhosh-Sellavel Will you be able to look at these proposals today?

[allroundexperts]

Taking over as C+

[allroundexperts]

Expected result: isClientTheLeader() === false because TabB took over as the new leader

Actual result: isClientTheLeader() === true and both tabs think they are the leader

@tgolen Are you sure that both tabs become the leader? As far as I can see, only the active tab (Tab A) is becoming the leader.

[tgolen]

I am not 100% sure about that, no. It sounds like most of the proposals here are able to disprove that theory. It was just what I was able to observe, which isn't actually what was happening.

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[isabelastisser]

@tgolen, do I need to assign a contributor to this issue since it's external, or are you going to work on this one?

[ShridharGoel]

@isabelastisser @tgolen My proposal is available for review: #40044 (comment)

[melvin-bot]

@tgolen, @isabelastisser Whoops! This issue is 2 days overdue. Let's get this updated quick!

[isabelastisser]

@allroundexperts, please complete the checklist below. Thanks!

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• The PR that introduced the bug has been identified. Link to the PR:

• The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:

• A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:

• Determine if we should create a regression test for this bug.

• If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

• [@isabelastisser] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[allroundexperts]

@allroundexperts, please complete the checklist below. Thanks!

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• The PR that introduced the bug has been identified. Link to the PR:
• The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• Determine if we should create a regression test for this bug.
• If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@isabelastisser] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

PR hasn't been merged yet @isabelastisser

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[danieldoglas]

I just saw that there's a fixed PR for this, but I came here to say that my client was stuck and showing a lot of [info] [Pusher] Received updates, but ignoring it since this is not the active client - ".

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.73-7 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

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

If no regressions arise, payment will be issued on 2024-05-22. 🎊

For reference, here are some details about the assignees on this issue:

• @Skakruk requires payment (Needs manual offer from BZ)

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@tgolen] The PR that introduced the bug has been identified. Link to the PR:
• [@tgolen] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@tgolen] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@Skakruk] Determine if we should create a regression test for this bug.
• [@Skakruk] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@isabelastisser] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[tgolen]

This wasn't really a regression. It was more just a flaw with the original feature. I don't think this needs the checklist filled out or any regression tests created.

[isabelastisser]

Sounds good. Thanks @tgolen! I will process the payments on May 22.

[melvin-bot]

Payment Summary

Upwork Job

• Contributor: @Skakruk paid $250 via Upwork
• C+ review: @allroundexperts paid $250 via Upwork

BugZero Checklist (@isabelastisser)

• I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
• I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants/1778173291530903552/hired)
• I have paid out the Upwork contracts or cancelled the ones that are incorrect
• I have verified the payment summary above is correct

[isabelastisser]

@Skakruk, can you please share your Upwork profile?

[Skakruk]

@isabelastisser here is my profile https://www.upwork.com/freelancers/~013b95670960c26f03

[isabelastisser]

Thank you! all set.

[allroundexperts]

@isabelastisser I get paid through ND. Can you please update the summary? Thanks!

[isabelastisser]

Payment Summary

Upwork Job

Contributor: @Skakruk paid $250 via Upwork
C+ review: @allroundexperts paid $250 via NewDot. PENDING PAYMENT

[JmillsExpensify]

$250 approved for @allroundexperts