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

Fix workspace not created after deeplinking to newDot #54992

Merged
merged 3 commits into from
Jan 14, 2025
Merged

Fix workspace not created after deeplinking to newDot #54992

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fcaf593
prevent api is called in deactive tab
nkdengineer Jan 14, 2025
c62c49e
fix lint error.
nkdengineer Jan 14, 2025
c590316
remove the appName
nkdengineer Jan 14, 2025
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
30 changes: 25 additions & 5 deletions src/libs/ActiveClientManager/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@
*/
import {Str} from 'expensify-common';
import Onyx from 'react-native-onyx';
import * as ActiveClients from '@userActions/ActiveClients';
import {isOpeningRouteInDesktop, resetIsOpeningRouteInDesktop} from '@libs/Browser/index.website';
import {setActiveClients} from '@userActions/ActiveClients';
import ONYXKEYS from '@src/ONYXKEYS';
import type {Init, IsClientTheLeader, IsReady} from './types';

Expand All @@ -16,6 +17,7 @@ let resolveSavedSelfPromise: () => void;
const savedSelfPromise = new Promise<void>((resolve) => {
resolveSavedSelfPromise = resolve;
});
let beforeunloadListenerAdded = false;

/**
* Determines when the client is ready. We need to wait both till we saved our ID in onyx AND the init method was called
Expand All @@ -40,7 +42,7 @@ Onyx.connect({

// Save the clients back to onyx, if they changed
if (removed) {
ActiveClients.setActiveClients(activeClients);
setActiveClients(activeClients);
}
},
});
Expand Down Expand Up @@ -70,19 +72,37 @@ const isClientTheLeader: IsClientTheLeader = () => {
const cleanUpClientId = () => {
isPromotingNewLeader = isClientTheLeader();
activeClients = activeClients.filter((id) => id !== clientID);
ActiveClients.setActiveClients(activeClients);
setActiveClients(activeClients);
};

const removeBeforeUnloadListener = () => {
if (!beforeunloadListenerAdded) {
return;
}
beforeunloadListenerAdded = false;
window.removeEventListener('beforeunload', cleanUpClientId);
};

/**
* Add our client ID to the list of active IDs.
* We want to ensure we have no duplicates and that the activeClient gets added at the end of the array (see isClientTheLeader)
*/
const init: Init = () => {
removeBeforeUnloadListener();
activeClients = activeClients.filter((id) => id !== clientID);
activeClients.push(clientID);
ActiveClients.setActiveClients(activeClients).then(resolveSavedSelfPromise);
setActiveClients(activeClients).then(resolveSavedSelfPromise);

window.addEventListener('beforeunload', cleanUpClientId);
beforeunloadListenerAdded = true;
window.addEventListener('beforeunload', () => {
// When we open route in desktop, beforeunload is fired unexpectedly here.
// So we should return early in this case to prevent cleaning the clientID
if (isOpeningRouteInDesktop()) {
resetIsOpeningRouteInDesktop();
return;
}
cleanUpClientId();
});
Comment on lines +97 to +105
Copy link
Contributor

Choose a reason for hiding this comment

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

The addEventListener is adding a new function everytime, and the removeEventListener won't remove the function given that cleanUpClientId doesn't exists. Probably that's why you need an extra check.
The Add/RemoveEventListener must receive the same function instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gedu So we can move this into cleanUpClientId function, right?

if (isOpeningRouteInDesktop()) {
    resetIsOpeningRouteInDesktop();
    return;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

It depends, if doing

window.addEventListener('beforeunload', cleanUpClientId);

desktop doesn't fire beforeunload, probably is not needed, but if just to really make sure, maybe yes, can be inside of cleanUpClientId

};

export {init, isClientTheLeader, isReady};
14 changes: 12 additions & 2 deletions src/libs/Browser/index.website.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import CONST from '@src/CONST';
import ROUTES from '@src/ROUTES';
import type {GetBrowser, IsChromeIOS, IsMobile, IsMobileChrome, IsMobileSafari, IsMobileWebKit, IsSafari, OpenRouteInDesktopApp} from './types';

let isOpenRouteInDesktop = false;
/**
* Fetch browser name from UA string
*
Expand All @@ -29,7 +30,7 @@ const getBrowser: GetBrowser = () => {
}
}

browserName = match[1] ?? navigator.appName;
browserName = match[1];
return browserName ? browserName.toLowerCase() : CONST.BROWSER.OTHER;
};

Expand Down Expand Up @@ -113,8 +114,17 @@ const openRouteInDesktopApp: OpenRouteInDesktopApp = (shortLivedAuthToken = '',
document.body.removeChild(iframe);
}, 0);
} else {
isOpenRouteInDesktop = true;
window.location.href = expensifyDeeplinkUrl;
}
};

export {getBrowser, isMobile, isMobileSafari, isMobileWebKit, isSafari, isMobileChrome, isChromeIOS, openRouteInDesktopApp};
const isOpeningRouteInDesktop = () => {
return isOpenRouteInDesktop;
};

const resetIsOpeningRouteInDesktop = () => {
isOpenRouteInDesktop = false;
};

export {getBrowser, isMobile, isMobileSafari, isMobileWebKit, isSafari, isMobileChrome, isChromeIOS, openRouteInDesktopApp, isOpeningRouteInDesktop, resetIsOpeningRouteInDesktop};
Loading