Fix infinite spinner in ND when navigating from OD #38567
Conversation
melvin-bot
bot
requested review from
eVoloshchak
and removed request for
a team
|
@eVoloshchak 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] |
Reviewer Checklist
Screenshots/VideosAndroid: NativeN/A Android: mWeb ChromeScreen.Recording.2024-03-19.at.4.23.52.PM.moviOS: NativeN/A iOS: mWeb SafariScreen.Recording.2024-03-19.at.4.20.12.PM.movMacOS: Chrome / SafariScreen.Recording.2024-03-19.at.4.16.44.PM.movMacOS: DesktopScreen.Recording.2024-03-19.at.4.19.17.PM.mov |
|
BUG Screen.Recording.2024-03-19.at.4.25.44.PM.movScreen.Recording.2024-03-19.at.4.22.26.PM.mov |
|
The following transition link seems to be broken as well. 
|
You mean it still shows loading spinner on the web? |
It's possible we need to use |
Shows a not found page. |
I pushed changes where I apply should skip to all platforms. Could you please check it again on native? |
|
Will check & get back. |
|
@mountiny This is something to save as follow up. I looked at the place where this function // If the app is opened from a deep link, get the reportID (if exists) from the deep link and navigate to the chat report
Linking.getInitialURL().then((url) => {
setInitialUrl(url);
Report.openReportFromDeepLink(url ?? '');
});
// Open chat report from a deep link (only mobile native)
Linking.addEventListener('url', (state) => {
Report.openReportFromDeepLink(state.url);
})In both cases the comments mention that it's for reports and the function itself indicates it's for reports. Somehow inside this function we are going through all possible urls not only reports. It may not be visible because in The check is broken in the issue linked above because transition route is removed before this function wants to navigate for the second time. I would say we should limit this function to handle only the reports, as comments say but this is something we need to test. There may be some side effects we are not aware. |
No luck. Still getting the same infinite loader on native. |
|
@allroundexperts Thanks for testing! That's weird. I will do some more investigation. |
|
Okay next batch of informations 😄 The function that is responsible for closing the transition page (loader) and opening desired page looks like that: function setUpPoliciesAndNavigate(session: OnyxEntry<OnyxTypes.Session>) {
const currentUrl = getCurrentUrl();
if (!session || !currentUrl || !currentUrl.includes('exitTo')) {
return;
}
const isLoggingInAsNewUser = !!session.email && SessionUtils.isLoggingInAsNewUser(currentUrl, session.email);
const url = new URL(currentUrl);
const exitTo = url.searchParams.get('exitTo') as Route | null;
// Approved Accountants and Guides can enter a flow where they make a workspace for other users,
// and those are passed as a search parameter when using transition links
const policyOwnerEmail = url.searchParams.get('ownerEmail') ?? '';
const makeMeAdmin = !!url.searchParams.get('makeMeAdmin');
const policyName = url.searchParams.get('policyName') ?? '';
// Sign out the current user if we're transitioning with a different user
const isTransitioning = Str.startsWith(url.pathname, Str.normalizeUrl(ROUTES.TRANSITION_BETWEEN_APPS));
const shouldCreateFreePolicy = !isLoggingInAsNewUser && isTransitioning && exitTo === ROUTES.WORKSPACE_NEW;
if (shouldCreateFreePolicy) {
createWorkspaceWithPolicyDraftAndNavigateToIt(policyOwnerEmail, policyName, true, makeMeAdmin);
return;
}
if (!isLoggingInAsNewUser && exitTo) {
Navigation.waitForProtectedRoutes()
.then(() => {
// We must call goBack() to remove the /transition route from history
Navigation.goBack();
Navigation.navigate(exitTo);
})
.then(endSignOnTransition);
}
}As you may see is starts with:
And this functions returns empty string for native platforms: So I guess that opening this link has never worked for native, even before ideal-nav-v2 because we have this guard later const currentUrl = getCurrentUrl();
if (!session || !currentUrl || !currentUrl.includes('exitTo')) {
return;
}The questions is if this PR should fix it or should it be outside of the scope. For me it feels like something different and should be handled separately. Let me know what do you think |
|
Only @mountiny can answer this! |
|
@allroundexperts @adamgrzybowski lets focus on fixing the desktop/ web redirect and the look into the native platforms redirects too
Which redirect you mean? I think deeplinks definitely worked but it might be true that the transition is not built for native, I think we would want users to go to mWeb and then they can openthe native app if they want @allroundexperts do the transitions work in mWeb? |
Yup, they work well on web / mweb. |
|
@adamgrzybowski Let's revert to your previous web only commit so that there are no side effects. |
This type of links This url is the one that should be result of the But it's an empty string on native Without this information it just return early and don't navigate and as a result we stay on the transition page with spinner. What I did noticed that this URL is present in params in the navigation state so there should be a way to get this information. I could look for someone from SWM to handle this if you want to create and issue for that. cc: @mountiny |
This reverts commit 7488983.
Done |
|
@mountiny looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 1.4.55-0 🚀
|
|
Not an Emergency the tests were passing |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 1.4.55-0 🚀
|
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 1.4.55-3 🚀
|
Details
Fixed Issues
$ #37332
PROPOSAL:
Tests
Offline tests
QA Steps
Precondition: user is logged in OD with an account that has a existing workspace without a bank account setup
OR
Click the message bubble in the bottom right
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
web.mov
MacOS: Desktop
desktop.mov