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.

Sign up for GitHub

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

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

Merged
merged 5 commits into from
May 14, 2024
Merged

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

Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8f080a8
Keep original isClientTheLeader value on page refresh
Skakruk May 13, 2024
d060f0b
Linting
Skakruk May 13, 2024
9a6c5d5
Linting
Skakruk May 13, 2024
850cb06
Change var name and add comment
Skakruk May 13, 2024
c185bdd
Update comment
Skakruk May 14, 2024
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
27 changes: 18 additions & 9 deletions src/libs/ActiveClientManager/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,25 @@ Onyx.connect({
},
});

let isPromotingNewLeader = false;

/**
* The last GUID is the most recent GUID, so that should be the leader
*/
const isClientTheLeader: IsClientTheLeader = () => {
// keep reporting currently leading client on page refresh
// when clientID is already removed from activeClients list
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// keep reporting currently leading client on page refresh
// when clientID is already removed from activeClients list
// When a new leader is being promoted, there is a small period of time when the current leader's clientID has been removed from the activeClients list due to asynchronous operations, but the new leader hasn't officially taken over yet. This can lead to a moment when the page is refreshed where multiple leaders are being reported. This early return here will prevent that from happening by keeping the current leader as the "active leader" until the other leader is fully promoted.

Please be sure that comments are all proper English and do a thorough job of explaining the purpose of the logic. Also, even though I've added this suggestion, you'll need to split it up into multiple lines (I suggest using our line max length setting for 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.

Thank you, I've updated the comment.

if (isPromotingNewLeader) {
return true;
}

const lastActiveClient = activeClients.length && activeClients[activeClients.length - 1];

return lastActiveClient === clientID;
};

const cleanUpClientId = () => {
isPromotingNewLeader = isClientTheLeader();
activeClients = activeClients.filter((id) => id !== clientID);
ActiveClients.setActiveClients(activeClients);
};
Expand All @@ -62,13 +80,4 @@ const init: Init = () => {
window.addEventListener('beforeunload', cleanUpClientId);
};

/**
* The last GUID is the most recent GUID, so that should be the leader
*/
const isClientTheLeader: IsClientTheLeader = () => {
const lastActiveClient = activeClients.length && activeClients[activeClients.length - 1];

return lastActiveClient === clientID;
};

export {init, isClientTheLeader, isReady};
Loading