Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.53k stars 2.88k forks source link

[$750] Desktop - "Hmm... it's not here" is displayed when a profile deeplink is opened two or more times #29372

Closed izarutskaya closed 10 months ago

izarutskaya commented 1 year ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Found when executing PR : https://github.com/Expensify/App/pull/29277

Version Number: v1.3.81-6

Reproducible in staging?: Y

Reproducible in production?: Y

If this was caught during regression testing, add the test name, ID and link from TestRail:

Email or phone of affected tester (no customers):

Logs: https://stackoverflow.com/c/expensify/questions/4856

Expensify/Expensify Issue URL:

Issue reported by: Applause-Internal Team

Slack conversation: @

Action Performed:

Precondition: User A and User B should have no prior chat history

As User A:

  1. Open a tab and sign in as User A
  2. Click on 'Profile avatar' > Share code > Copy URL to clipboard

As User B:

  1. Open staging.new.expensify.com and sign in as User B
  2. Paste the URL into the address bar
  3. Press enter
  4. In the ''Open New Expensify'' modal, click on the CTA button to open the app
  5. Close the profile page
  6. Go back to the web page and paste the profile link for a second time
  7. Repeat step 4

Expected Result:

There should be no errors when opening the deeplink from the same profile page multiple times.

Actual Result:

The user encounters a "page not found" error when attempting to open the profile page deeplink two or more times in Desktop app.

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Screenshots/Videos

Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop https://github.com/Expensify/App/assets/115492554/33adb5ef-b628-45da-88fa-fc417dbd4d73 [utest-dl.s3.amazonaws.com_12102_26469_432782_6233436_bugAttachment_Bug6233436_1697049112244%21Profile_page_-_Desktop.log_X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Date=20231011T195222Z&X-Amz-SignedHeaders=host&X-Amz-Expires=86400&X-Amz.txt](https://github.com/Expensify/App/files/12874038/utest-dl.s3.amazonaws.com_12102_26469_432782_6233436_bugAttachment_Bug6233436_1697049112244.21Profile_page_-_Desktop.log_X-Amz-Algorithm.AWS4-HMAC-SHA256.X-Amz-Date.20231011T195222Z.X-Amz-SignedHeaders.host.X-Amz-Expires.86400.X-Amz.txt) [View all open jobs on GitHub](https://github.com/Expensify/App/issues?q=is%3Aopen+is%3Aissue+label%3A%22Help+Wanted%22)
Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~019231ee47ef138a05
  • Upwork Job ID: 1712194879740461056
  • Last Price Increase: 2023-10-27
  • Automatic offers:
    • situchan | Contributor | 27546164
melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @NicMendonca (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~019231ee47ef138a05

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

melvin-bot[bot] commented 1 year ago

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

akamefi202 commented 1 year ago

Not reproducible.

NicMendonca commented 1 year ago

@izarutskaya can you still reproduce this issue?

izarutskaya commented 1 year ago

Yes, @NicMendonca, we can reproduce this still on Version 1.3.83-10 (1.3.83-10)

https://github.com/Expensify/App/assets/115492554/b6764432-08be-428d-8034-015ef62abca9

https://github.com/Expensify/App/assets/115492554/0f3d231f-babb-4398-bc83-a8a9e47c4b6d

NicMendonca commented 1 year ago

Waiting for proposals

melvin-bot[bot] commented 1 year ago

πŸ“£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πŸ’Έ

NicMendonca commented 1 year ago

@eVoloshchak should we adjust bounty?

eVoloshchak commented 1 year ago

@NicMendonca, i think we should, since there aren't any proposals

melvin-bot[bot] commented 1 year ago

Upwork job price has been updated to $750

barttom commented 1 year ago

Hey! I'm Bartek from Callstack - an expert contributor group. I’d like to work on this issue. I'm gonna investigate that and prepare proposal

melvin-bot[bot] commented 1 year ago

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

NicMendonca commented 1 year ago

Thank you @barttom πŸ₯³

barttom commented 1 year ago

@izarutskaya Are You still able to reproduce that? I've tried a few times and works fine for me. I used staging on the browsers and current main branch for desktop

https://github.com/Expensify/App/assets/7682108/a7c27a3f-16ac-41d5-99eb-96d08fc4f78c

ikevin127 commented 1 year ago

I can still reproduce this on latest staging release of the desktop app Version 1.3.90-2 (1.3.90-2) downloaded via: https://staging.new.expensify.com/NewExpensify.dmg

The thing is that it can be reproduced but not always exactly as stated in the steps of the OP, the interval of when the error occurs differs. Sometimes it happens on 2nd - 3rd try of deep link, sometimes you have to open the link 6-7 times.

I noticed it occurs more often if the left side drawer which the shared link opens, is dismissed (closed) on the desktop app, when re-opening the deep link from web again it's more likely to get the not found page (more context below on debugging).

The equivalent to the not found page from staging that I get on local development of the desktop app is the video shown above with the infinite loading indicator. So instead of the not found page we see the infinite loading indicator on local development with latest main branch.

Debugging: From what I've been able to research so far, my guess is that it's somewhat related to the fact that before the drawer opened by the shared link is triggered, the ReportScreenWrapper's navigator does not have the reportID prop initially but it does get set by ReportScreenIDSetter eventually which in the case of this issue might cause the not found page seen on staging or the infinite loading indicator seen on local dev.

This is just a pointer I wanted to share which might help with debugging. Good luck!

barttom commented 1 year ago

@ikevin127 thanks for the tip, I'll keep debugging it then.

melvin-bot[bot] commented 1 year ago

@eVoloshchak @NicMendonca this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

studentofcoding commented 1 year ago

Proposal

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

Desktop - "Hmm... it's not here" is displayed when a profile deeplink is opened two or more times

What is the root cause of that problem?

The root cause of the problem is that the Desktop App deeplink is not correctly handling the routing for profile pages on desktop/main.js

    app.on('will-finish-launching', () => {
        app.on('open-url', (event, url) => {
            event.preventDefault();
            const urlObject = new URL(url);
            deeplinkUrl = `${APP_DOMAIN}${urlObject.pathname}${urlObject.search}${urlObject.hash}`;

            if (browserWindow) {
                browserWindow.loadURL(deeplinkUrl);
                browserWindow.show();
            }
        });
    });

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

In the desktop/main.js file, we need to modify the app.on('open-url', ...) event handler to specifically handle profile page URLs if user open it

Here's the updated code:

app.on('will-finish-launching', () => {
    app.on('open-url', (event, url) => {
        event.preventDefault();
        const urlObject = new URL(url);
        // Check if the URL is a profile page URL
        if (urlObject.pathname.startsWith('/a/')) {
            // If it is, load the profile page URL directly
            deeplinkUrl = url;
        } else {
            // Otherwise, use the existing logic
            deeplinkUrl = `${APP_DOMAIN}${urlObject.pathname}${urlObject.search}${urlObject.hash}`;
        }

        if (browserWindow) {
            browserWindow.loadURL(deeplinkUrl);
            browserWindow.show();
        }
    });
});

This code checks if the URL is a profile page URL (i.e., it starts with /a/). If it is, it loads the profile page URL directly. Otherwise, it uses the existing logic to load the URL.

What alternative solutions did you explore? (Optional)

None

Result

100% (10 out of 10) load the profile

https://github.com/Expensify/App/assets/20473526/5b86e03b-4cb8-47c1-bcd0-31490a3d6d31

@ikevin127 @eVoloshchak @NicMendonca

ikevin127 commented 1 year ago

@studentofcoding Your solution does not solve the problem, it doesn't do anything actually. I tested it both on a staging and local development builds and for staging we still get the not found page while on local we still get the infinite loop loading indicator:

MacOS: Desktop - staging build: https://github.com/Expensify/App/assets/56457735/3ac2ac55-bfc4-49c2-95d9-3ee070f381e6 - development build: https://github.com/Expensify/App/assets/56457735/cbea76a8-752a-41ab-881c-06f0ef3b2a45

You're close to the root but not quite there, the issue's root is not coming from the electron main.js file nor it can be fixed from there.

The issue is much more complex, hence why the bounty for this issue was raised and why it takes longer than usual to solve it, not to talk about the different behaviour between staging build vs local development, I think a solution is needed which would handle both staging / local issues.

I'm still debugging this and if I'll figure it out will come up with a proposal soon.

ikevin127 commented 1 year ago

Proposal

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

The user encounters a "page not found" error when attempting to open the profile page deeplink two or more times in Desktop app.

What is the root cause of that problem?

The reason why we get the page not found error on staging desktop and infinite loading spinner on local development desktop is because the desktop app is calling navigate with transition route and this action is not handled by the navigator.

The way the flow was designed to work for the desktop app:

Important: transition route is used to deeplink web to desktop in this case, it was not designed to actually be called by the navigator once we're in the desktop app.

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

https://github.com/Expensify/App/blob/414d5913999f69d83ab08b6ae6e03325c9ede549/src/libs/actions/Report.js#L1935-L1943

We have to make sure that the transition route passed from the main electron process is NOT called by the navigator once we are transitioned into the web app, for this we check:

if these conditions are met then we return meaning we not call the react navigation with the transition route which causes the issue on staging / local desktop app.

             if (route === ROUTES.CONCIERGE) { 
                 navigateToConciergeChat(true); 
                 return; 
             }

                // If the platform is desktop and route is `transition` we don't want to navigate
                // to the route because it will throw page not found on staging and
                // infinite loading indicator on local development.
                // See issue: https://github.com/Expensify/App/issues/29372
+               if (getPlatform() === CONST.PLATFORM.DESKTOP && route.includes(ROUTES.TRANSITION_BETWEEN_APPS) {
+                   return;
+               }

             Navigation.navigate(route, CONST.NAVIGATION.TYPE.PUSH);

Videos

MacOS: Desktop (staging / local) - staging https://github.com/Expensify/App/assets/56457735/5428f539-2b24-4149-8893-7516173b95c6 - local dev https://github.com/Expensify/App/assets/56457735/8bc8be14-30b2-41de-bc61-bf3811926968
NicMendonca commented 1 year ago

@eVoloshchak proposals for you ^

mvtglobally commented 1 year ago

Issue not reproducible during KI retests. (First week)

melvin-bot[bot] commented 1 year ago

πŸ“£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πŸ’Έ

barttom commented 1 year ago

I think we can go with @ikevin127 solution, can't figure out anything else now :) After testing I've had the same results as on videos.

eVoloshchak commented 1 year ago

@ikevin127, could you check if this is still reproducible? I'm unable to reproduce the issue in both dev and prod versions of the Desktop app

https://github.com/Expensify/App/assets/9059945/e47336fa-a167-4cd3-af26-6680c6c12e3d

ikevin127 commented 1 year ago

@eVoloshchak Yes, this is still reproducible. Just checked with the latest main, both locally (loading indicator loop) and also with a staging build (not found page).

It just doesn't happen right away (like in action performed), sometimes it can take a few tries as I mentioned in my proposal, it depends on a few factors.

For example in videos below:

MacOS: Desktop (staging / local) - staging https://github.com/Expensify/App/assets/56457735/b30aefb1-94fe-4963-a68e-278790a31005 - local dev https://github.com/Expensify/App/assets/56457735/c51f270a-eddc-4d47-8a50-3341288e013b
melvin-bot[bot] commented 1 year ago

πŸ“£ @eVoloshchak Please request via NewDot manual requests for the Reviewer role ($750)

melvin-bot[bot] commented 1 year ago

@barttom @eVoloshchak @NicMendonca this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

ikevin127 commented 1 year ago

@barttom I noticed you've been assigned to this. Are you handling this issue based on my proposal or are you working on your own proposal for this ?

barttom commented 1 year ago

@ikevin127 I think Your solution is great, didn't find any better. Based on that proposal is Yours, shouldn't Youb implement it? If You want me to handle that, just say a word, it's not a problem for me at all :)

ikevin127 commented 1 year ago

@barttom Sure, the reason I made the proposal is because I want to handle this issue. I was asking because I seen you've been assigned by @NicMendonca and did not know why or whether or not you will be taking on this.

eVoloshchak commented 1 year ago

Still can't reproduce this even after trying for 10 minutes straight. Throttling the network speed also doesn't do anything. I think this might be related to performance, @ikevin127, which machine are you using to reproduce the issue?

ikevin127 commented 1 year ago

@eVoloshchak I used a MacBook Air - M1 2020, 8GB RAM on Sonoma 14.0 to reproduce this.

What are you using ?

@izarutskaya and @barttom were also able to reproduce this, maybe they can help with info about their machines.

barttom commented 1 year ago

so mine is Apple M1 Pro 32GB RAM and macOS 13.5.1 and I reproduced that a few times. It's tricky; when it starts working - never breaks again for me. I was able to reproduce that by creating a new pair of users.

melvin-bot[bot] commented 1 year ago

@barttom, @eVoloshchak, @NicMendonca Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

eVoloshchak commented 1 year ago

My setup is similar (M1 pro, but I'm using macOS14), but still cannot reproduce this one even when creating new accounts Asked on Slack for this to be reassigned, maybe others would be able to reproduce the issue

melvin-bot[bot] commented 1 year ago

πŸ“£ @eVoloshchak Please request via NewDot manual requests for the Reviewer role ($750)

melvin-bot[bot] commented 1 year ago

πŸ“£ @situchan πŸŽ‰ An offer has been automatically sent to your Upwork account for the Contributor role πŸŽ‰ Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review πŸ§‘β€πŸ’» Keep in mind: Code of Conduct | Contributing πŸ“–

melvin-bot[bot] commented 1 year ago

@barttom @NicMendonca @situchan this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

Thanks!

melvin-bot[bot] commented 1 year ago

Current assignee @situchan is eligible for the Internal assigner, not assigning anyone new.

melvin-bot[bot] commented 12 months ago

@barttom, @NicMendonca, @situchan Whoops! This issue is 2 days overdue. Let's get this updated quick!

situchan commented 12 months ago

updating today

situchan commented 12 months ago

After some random testing, I reproduced this on web rather than desktop. I think the issue is not limited to desktop app. And it seems related to navigation logic.

not found
ikevin127 commented 12 months ago

What are the parameters of reproducing this issue on web, what were the actions performed exactly ?

I'd argue that the reported not found page on web has nothing to do with the RCA of this web to desktop deep-linking issue. If it can be determined precisely that both have the same root cause then I'll update my proposal if needed, otherwise I'd keep the 2 issues separated.

Update:

Just checked with latest main, ran npm install then built desktop app on both dev and stg and I can still reproduce the same behaviour: on dev we get infinite loading spinner and on stg build we get page not found for the same root cause.

MacOS: Desktop (STG / DEV) - STG https://github.com/Expensify/App/assets/56457735/91a4f87b-cb50-4594-84d5-6a6cac87ceae - DEV https://github.com/Expensify/App/assets/56457735/3b143914-6725-4a72-ae58-0319804aef52

Details for reviewer:

melvin-bot[bot] commented 11 months ago

@barttom, @NicMendonca, @situchan Whoops! This issue is 2 days overdue. Let's get this updated quick!

situchan commented 11 months ago

still we're trying to have constant repro step

melvin-bot[bot] commented 11 months ago

@barttom, @NicMendonca, @situchan Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] commented 11 months ago

@barttom, @NicMendonca, @situchan Huh... This is 4 days overdue. Who can take care of this?