[HOLD #20908] [$1000] Dev: Android - When returning from a thread to the main chat and clicking on the back button, the thread message is opened again

kbecciv 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!

Action Performed:

Open a chat
Send a message
Click on "Reply in thread"
Click on "From user1"
Return to the main chat
Click on the back button from main chat

Expected Result:

Clicking the back button from the main chat should navigate back to the main page

Actual Result:

When returning from a thread to the main chat and clicking on the back button, the thread message is opened again instead of going back to the main page

Workaround:

Unknown

Platforms:

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

[x] Android / native
[ ] Android / Chrome
[ ] iOS / native
[ ] iOS / Safari
[ ] MacOS / Chrome / Safari
[ ] MacOS / Desktop

Version Number: v1.3.27-6

Reproducible in staging?: n/a

Reproducible in production?: n/a

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

Notes/Photos/Videos: Any additional supporting documentation

https://github.com/Expensify/App/assets/93399543/5cf16182-d2e1-4b00-92af-997370d28f21

Expensify/Expensify Issue URL:

Issue reported by: @ayazhussain79

Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1686745578325549

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~018d085ef1fee5c5db
Upwork Job ID: 1673308483340644352
Last Price Increase: 2023-07-17

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

[x] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
[x] This bug is not a duplicate report (check E/App issues and #expensify-bugs)
- If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
[x] This bug is reproducible using the reproduction steps in the OP. S/O
- If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
- If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
[x] This issue is filled out as thoroughly and clearly as possible
- Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
[x] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

CortneyOfstad commented 1 year ago

So watching the video, I'm not sure if this is actually a bug. When clicking on "From Progress" (the name of the chat) in the header, it would make sense to bring you back to that chat.

I guess I don't see how this is a bug, as it looks like it is working as intended.

ayazhussain79 commented 1 year ago

When you want to go to chat history you need to press back button 3,4 times to go there. When you click on the "From" link you will go to the main chat then when you press the back button from the main chat you should go to the chat history.

CortneyOfstad commented 1 year ago

Oooh! Sorry for the confusion on my part! I was able to recreate this on my end, and experienced the additional 3-4 clicks to get back to the full chat list.

Going to get eyes on this!

melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~018d085ef1fee5c5db

melvin-bot[bot] commented 1 year ago

Current assignee @CortneyOfstad is eligible for the External assigner, not assigning anyone new.

melvin-bot[bot] commented 1 year ago

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

Julesssss commented 1 year ago

I agree this seems like a navigation hierarchy bug. On Android we should not be able to infinitely add pages to our hierarchy. Tapping 'From progress' should pop the current page, not add a new one to the stack

melvin-bot[bot] commented 1 year ago

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

Julesssss commented 1 year ago

Not overdue. we're awaiting proposals.

CortneyOfstad commented 1 year ago

Heading OoO for the week (back July 10) so re-assigning this in the meantime 👍

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

[ ] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
[ ] This bug is not a duplicate report (check E/App issues and #expensify-bugs)
- If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
[ ] This bug is reproducible using the reproduction steps in the OP. S/O
- If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
- If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
[ ] This issue is filled out as thoroughly and clearly as possible
- Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
[ ] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

Julesssss commented 1 year ago

Bug Triage checklist was already completed

Julesssss commented 1 year ago

@CortneyOfstad maybe we should think about doubling the bounty here?

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? 💸

Nodebrute commented 1 year ago

@Julesssss @abdulrahuman5196 Question. If we open an already-created thread and navigate to the parent report from a header. What is the expected behavior of pressing back?

Nodebrute commented 1 year ago

Proposal

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

When returning from a thread to the main chat and clicking on the back button, the thread message is opened again

What is the root cause of that problem?

here the enforce fallback is set to false. ROUTES.HOME will only work if fallback is set to true.

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

change this line to
onNavigationMenuButtonClicked={() => Navigation.goBack(ROUTES.HOME, true, true)} here I have set the fallback to true so no matter wherever we are in stack pressing back of report will always take you back on main page. This will also fix the issue we are currently viewing task page

P.s double price will be appreciated 😂😂 Results

https://github.com/Expensify/App/assets/93134676/8d12b735-02ce-4f4d-81b7-ce1354ef744a

https://github.com/Expensify/App/assets/93134676/8b2d5316-fce4-40ab-a85c-8baead9d73f3

What alternative solutions did you explore? (Optional)

Julesssss commented 1 year ago

If we open an already-created thread and navigate to the parent report from a header. What is the expected behavior of pressing back?

We should go back again to the LHN. Navigation should be hierarchy-based.

Julesssss commented 1 year ago

@abdulrahuman5196 please take a look at this proposal. It looks good to me

abdulrahuman5196 commented 1 year ago

@Nodebrute From this proposal here https://github.com/Expensify/App/issues/21214#issuecomment-1620664217,

Its only fixing button press on the navigation header. But if we use the hardware back button the issue still exists.

cc: @Julesssss

Nodebrute commented 1 year ago

@abdulrahuman5196 I'll get back to you. I am working on this.

Nodebrute commented 1 year ago

@abdulrahuman5196 The solution is ready. I have tested it on all devices. Can you test it for regressions? I am confident it will not cause any regressions.

https://github.com/Expensify/App/assets/93134676/f97581fe-05db-4321-93d6-8af51835ba46

melvin-bot[bot] commented 1 year ago

@Julesssss @michaelhaxhiu @abdulrahuman5196 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!

Nodebrute commented 1 year ago

@abdulrahuman5196 @Julesssss The solution is quite complex as it involves a sensitive part of the navigation. We have to test it with all navigation cases. I request you to increase the price. Thank you.

Nodebrute commented 1 year ago

Proposal

https://github.com/Expensify/App/issues/20908 https://github.com/Expensify/App/issues/21214

Even though these two issues are completely separate, this solution will fix both of them. There may be other issues related to this as well.

You can test the same code for thread and concierge issue.

With this proposal I'll be addressing 2 separate issue. Kindly bear with me

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

Pressing back from reports should bring us back to LHN.

What is the root cause of that problem?

We are pushing screens into the navigation stack, going deeper into the stack. Another problem here is that if the user is on [mweb], they have to press the back button multiple times to exit the site.

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

First, I would like to mention a few problems that we have to deal with:

We can control the Android back button with the back button handler API provided by React Native. However, this approach does not work for iOS.
When we press the physical back button, we have to navigate through all the screens that were previously in the stack.
We have to address the differences between browsers and native platforms. Something that works well for native may cause issues for the web. And many more....

I propose resetting the state whenever lastRoute.name === CENTRAL_PANE_NAVIGATOR(Report). This means that no matter how deep we are in the stack, we will always be redirected to the main screen. This approach will not affect modals and will provide the same results as pressing the back button in the app. Resetting the state is acceptable. Back button will provide the same result on all platforms.

import * as Browser from '../../libs/Browser';

Add the following code here

        if (action.payload.name === NAVIGATORS.CENTRAL_PANE_NAVIGATOR) {
            // Check if the previous route in the stack is the Central Pane Navigator
            if (lastRoute.name === NAVIGATORS.CENTRAL_PANE_NAVIGATOR) {
              // Trigger root.reset when going back from Central Pane Navigator
              const homeRoute = _.get(root.getState(), ['routes', 0]);
              if (Browser.isMobileChrome() || Browser.isMobileSafari()) {
                root.navigate(homeRoute);
              }else{
                root.reset(state);
                return;
              }
            }
        }

Here, we are not resetting the state for mobile browsers. Resetting the state on mobile browsers may lead to unexpected results. We can either add other browsers to the condition or include an additional check to detect if it is a mobile browser.

You can test the above code. Of course, this is not the final code. Your input will be appreciated. We can discuss changes, best practices, and where we should add this. If you have any concerns feel free to ask.

Note: these are separate If statements.

We need to test this for all navigation cases. If you observe any unexpected behavior, kindly test it first without adding my solution. This way, we can confirm whether the bug is reproduced by my solution or not. Again this may be not the final code. This is just to test if Approach works or not.

Note:Before testing this code reload the page so we don't have anything in stack from previous code.

Results

https://github.com/Expensify/App/assets/93134676/4d5e9966-ca5a-4017-bf94-cc0d9aebffb1

https://github.com/Expensify/App/assets/93134676/b600f7b2-7f78-49c5-b2eb-2233b65b087b

https://github.com/Expensify/App/assets/93134676/0bf5f593-87b6-440a-8ebf-898632810481

https://github.com/Expensify/App/assets/93134676/729fe884-7e60-498b-bc5b-4f4107f7dd37

What alternative solutions did you explore? (Optional)

mountiny commented 1 year ago

@WoLewicki @adamgrzybowski could you check the proposal from @Nodebrute?

I believe we want to avoid making that navigate method too bloated and complicated. If there would be some easier way how to solve this that would be great.

Julesssss commented 1 year ago

This means that no matter how deep we are in the stack, we will always be redirected to the main screen.

I don't think this is quite the expected behaviour. We still want a multi-level navigational hierachy, so pressing back from a thread should take you up one level to the chat, rather than up 2 levels to the LHN.

LHN > Chat > Expense > Thread 1 > 2 > 3 > 4

back (Expense) 1 > 2 > 3

back (Chat) 1 > 2

back (LHN) 1

adamgrzybowski commented 1 year ago

Hey, let's clarify the expected behavior.

Currently, we are pushing every chat even if it is already on the stack.

e.g. imagine chats with deeplinks A -> B -> A In that case, the stack should be A, B, A Same for clicking A -> B -> A on the sidebar.

But for threads, we want to go back to the chat if it's already on the stack. So it would be

A -> Athread - the "from" button > A And now there should be only A on the stack?

What if the chat under the "from" is more than one back away from the thread screen?

e.g. A -> B -> Athread - the “from” button > and what now? Should we go to pop all the way to A or just push another A?

Also, what if the user opened a thread first Athread - the "from" button > A Maybe in that case we should do a replace so the user can't go back to the thread by pressing the browser back button

cc: @WoLewicki

Julesssss commented 1 year ago

Ah yeah good point, deep linking is the special case here.

adamgrzybowski commented 1 year ago

So essentially I think that what is happening now is expected general behavior for chats but if we want to deal with threads in some specific way to improve UX we can do it.

However, I would avoid doing it by adding more ifs in the navigate function if it's only for threads. There is no reason to create additional dependency on the structure of the app if not necessary.

abdulrahuman5196 commented 1 year ago

@adamgrzybowski Just thinking out load., What if, In case of "From A" header pressed in thread, Maybe we can pop the threadA screen from the stack if the previous screen is A?

If A-> threadA -> A it will become A if A -> B -> threadA -> A there won't be any change. same for different combination.

It seems to be pretty annoying experience IMO when we use the From header and try to return back to the LHN.

Nodebrute commented 1 year ago

@abdulrahuman5196 @Julesssss @abdulrahuman5196 should I update my proposal? What do we want to do here? Here's another thing we can do

If A -> thread a -> A then instead of navigating to A we can use "go Back" to go to A. This will not push A to screen instead it will go Back to first A which means when we press back from there we will go back to LHN.

but

If thread A -> A here another A will be added in the stack.

here we can conditionally check if we have A in stack then goBack else push it

WoLewicki commented 1 year ago

My opinions:

A -> Athread - the "from" button > A And now there should be only A on the stack?

Yes.

What if the chat under the "from" is more than one back away from the thread screen? e.g. A -> B -> Athread - the “from” button > and what now? Should we go to pop all the way to A or just push another A?

Push another A

Also, what if the user opened a thread first Athread - the "from" button > A Maybe in that case we should do a replace so the user can't go back to the thread by pressing the browser back button

Replace seems most reasonable since the "from" looks kinda like the back button to the main chat.

Nodebrute commented 1 year ago

@Julesssss @WoLewicki do we have only these three cases? Another question here

In case two, we are pushing A, which means pressing back will take us to the thread. So, why in case three do we want to replace? I think it's fine to go back to the thread in case three.

Nodebrute commented 1 year ago

@abdulrahuman5196 @Julesssss I think something like this will work.

function childNavigation(parentID){
    const rootState = navigationRef.getRootState();
    const routes = rootState.routes;
    const secondLastRoute = routes.length > 1 ? routes[routes.length - 2] : null;
    const secondLastRouteReportID = secondLastRoute?.params?.params?.reportID 
    if(+secondLastRouteReportID === parentID){
        goBack()
    } else{
        navigate(ROUTES.getReportRoute(parentID))
    }
}

I am not sure about the third case of the replace, whether we should implement it or not.

mountiny commented 1 year ago

I agree with the behaviour suggested by @WoLewicki and @adamgrzybowski

Nodebrute commented 1 year ago

@abdulrahuman5196 @WoLewicki https://github.com/Expensify/App/issues/21214#issuecomment-1624163173 If something like this works for you, can I start working on the replace case?

Julesssss commented 1 year ago

But for threads, we want to go back to the chat if it's already on the stack. So it would be

I think we're on the same page here, but just to be sure does the following align with this statement?

If the user's path was LHN > Chat > Thread, after pressing back their nav stack would now be LHN > Chat
If they had deep-linked to the thread via LHN > Chat > ThreadA > ThreadB, after pressing back their new nav stack would be LHN > Chat > ThreadA

WoLewicki commented 1 year ago

For the second point, if they press browser back button or report's back button, they should be navigated the way you mentioned. We would only treat the "from" button in the special way, in this case replacing ThreadB with ChatB or pushing ChatB, as suggested above. Which option do you like better? Replacing seems more reasonable to me at this moment.

mountiny commented 1 year ago

Maybe we should bring this to slack with some examples in the app, its hard to exactly imagine thee just in text like this

@Nodebrute would you be able to bring this to the open source channel and highlight the two options with examples so we can decide which one is better aligned with our patterns

Nodebrute commented 1 year ago

For the second point, if they press browser back button or report's back button, they should be navigated the way you mentioned. We would only treat the "from" button in the special way, in this case replacing ThreadB with ChatB or pushing ChatB, as suggested above. Which option do you like better? Replacing seems more reasonable to me at this moment.

These options? @mountiny Done.

Nodebrute commented 1 year ago

So, We have few votes for Replace and 0 votes for push.....

We'll only control from button here. With this approach deep links will not be affected. we have 3 cases here and we can have 3 checks for them

Case 1

A -> Athread - the "from" button > A And now there should be only A on the stack?

Here we can add this check (+secondLastRouteReportID === parentID) then go Back.

Case 2

What if the chat under the "from" is more than one back away from the thread screen?

e.g. A -> B -> Athread - the “from” button > and what now? Should we go to pop all the way to A or just push another A?

Here we can simply push A here we don't need to add any check we can add it in last else of if statement.

Case 3

Also, what if the user opened a thread first Athread - the "from" button > A Maybe in that case we should do a replace so the user can't go back to the thread by pressing the browser back button

If the user has opened a thread and the last route in the stack is the "Home" route, we will replace that screen we can use this condition (secondLastRoute.name === 'Home'). This condition also applies to deep links. For example, if the user opens a thread, shares its link, User B opens the link and navigates to the parent from header, the last route of the thread will still be "Home".

This will work on all platforms. I welcome and appreciate any suggestions or improvements.

mountiny commented 1 year ago

I think this sounds good to me, if Jules agree we could go ahead with this i guess.

I will let you guys handle this now, tag me if needed thanks for discussion 🙏

Julesssss commented 1 year ago

@WoLewicki

...in this case replacing ThreadB with ChatB or pushing ChatB, as suggested above. Which option do you like better?

I mostly agree with replace, but I'd say we should go further and collapse the navigation stack for this case, rather than just the current page. This avoids the problem where pressing back shows pages lower in the navigation hierarchy and ensures the user doesn't get stuck having to press back more than a few times.

Starting here: LHN > ChatA > ThreadA > ThreadB

Instead of being in this state: LHN > ChatA > ThreadA > ChatB

...we would be in this state: LHN > ChatB

WoLewicki commented 1 year ago

Hmm I mean we could do a reset action then probably, but it often ends up with some weird behaviors of browser back/forward buttons. Still, we can try that solution.

On the other hand, the replace action is more predictable from the navigation's point of view and we already push all chats on the stack, so you can navigate between 2 reports all the time and have them pushed many many times on your stack.

Nodebrute commented 1 year ago

...we would be in this state: LHN > ChatB

It would make things more complex with browsers. Collapsing here could create the same issue we have with leaving threads. Only reset can work here then and my initial proposal was about resetting state

Julesssss commented 1 year ago

It almost sounds like we're limited here by React Navigation functionality here. Is that correct, or am I reading too much into the reset vs replace actions?

I'm mostly speaking for native Android navigation here, but resetting/clearing the navigation stack is more aligned with native mobile navigation IMO. The resoning being that deep links should simulate manual navigation.

When deep linking to a destination within your app’s task, any existing back stack for your app’s task is removed and replaced with the deep-linked back stack.

So tapping 'Chat with' is essentially starting a new flow where the user went from LHN to the chat. This guarentees that flows 2 and 3 (from this comment) are identical. Deep links have a consistent outcome regardless of the entry point: 'back' to get to the LHN, then another 'back' to exit the app (assuming the deep link is to a thread).

Nodebrute commented 1 year ago

@Julesssss what's the next move here?

Expensify / App