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.

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

[$250] mWeb/safari - Invoice - App shows confirmation page when swiping right after sending invoice #41940

Closed
1 of 6 tasks
kbecciv opened this issue May 9, 2024 · 31 comments
Closed
1 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@kbecciv
Copy link

kbecciv commented May 9, 2024

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


Version Number: 1.4.72-0
Reproducible in staging?: y
Reproducible in production?: y
Issue reported by: Applause - Internal Team

Action Performed:

  1. Go to staging.new.expensify.com
  2. Go to FAB > Send invoice.
  3. Enter amount and select a user.
  4. Create the invoice.
  5. In invoice chat, swipe the page to the right (iOS style).

Expected Result:

Invoice confirmation page will not appear when swiping right.

Actual Result:

Invoice confirmation page appears briefly when swiping right.

Workaround:

n/a

Platforms:

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

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6475968_1715262585623.RPReplay_Final1715262285.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01aea40b16c5d0e563
  • Upwork Job ID: 1790003603403919360
  • Last Price Increase: 2024-05-13
Issue OwnerCurrent Issue Owner: @jjcoffee
@kbecciv kbecciv added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels May 9, 2024
Copy link

melvin-bot bot commented May 9, 2024

Triggered auto assignment to @CortneyOfstad (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@kbecciv
Copy link
Author

kbecciv commented May 9, 2024

We think that this bug might be related to #wave-collect - Release 1

@kbecciv
Copy link
Author

kbecciv commented May 9, 2024

@CortneyOfstad FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors.

@melvin-bot melvin-bot bot added the Overdue label May 13, 2024
@VickyStash
Copy link
Contributor

Hi, I'm Viktoryia from Callstack - expert contributor group - and I would like to work on this issue.
cc @cristipaval

@cristipaval
Copy link
Contributor

@davidcardoza, I added LOW priority on this one, please let me know if you think another one fits better

@cristipaval cristipaval added the External Added to denote the issue can be worked on by a contributor label May 13, 2024
@melvin-bot melvin-bot bot changed the title mWeb/safari - Invoice - App shows confirmation page when swiping right after sending invoice [$250] mWeb/safari - Invoice - App shows confirmation page when swiping right after sending invoice May 13, 2024
Copy link

melvin-bot bot commented May 13, 2024

Job added to Upwork: https://www.upwork.com/jobs/~01aea40b16c5d0e563

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label May 13, 2024
Copy link

melvin-bot bot commented May 13, 2024

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

@cristipaval cristipaval removed the Help Wanted Apply this label when an issue is open to proposals by contributors label May 13, 2024
@VickyStash
Copy link
Contributor

I was able to reproduce the issue, but it looks like it doesn't relate specifically to Send Invoice flow, it's reproducible with Submit expense as well.

@VickyStash
Copy link
Contributor

I'm still working on the proposal.
The problem seems to occurs because the navigation history that Safari manages does not sync up perfectly with the navigation controlled by React Navigation.
The navigation from Confirmation page to Report page is handled by replace method. But the iOS Safari swipe back gesture can still access the browser's history stack and shows the previous (confirmation) screen during the swipe.

@CortneyOfstad
Copy link
Contributor

Thanks @VickyStash!

@melvin-bot melvin-bot bot added the Overdue label May 15, 2024
@cristipaval
Copy link
Contributor

Not overdue, Melv

@melvin-bot melvin-bot bot removed the Overdue label May 15, 2024
@VickyStash
Copy link
Contributor

I haven't had a lot of time today for this ticket, but I created a simple app to test similar navigation scenario and it looks like I had the same problem. So the main guess for now that's it can be navigation lib problem rather than the app bug.

@Pujan92
Copy link
Contributor

Pujan92 commented May 17, 2024

@CortneyOfstad To inform, I am OOO until 27th May. Either we can reassign to other C+ or else I will check post 27th May.

@melvin-bot melvin-bot bot added the Overdue label May 17, 2024
@CortneyOfstad
Copy link
Contributor

Going to reassign the C+ — thanks @Pujan92 and have a great time away!

@melvin-bot melvin-bot bot removed the Overdue label May 20, 2024
@CortneyOfstad CortneyOfstad added Overdue External Added to denote the issue can be worked on by a contributor and removed External Added to denote the issue can be worked on by a contributor labels May 20, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label May 20, 2024
Copy link

melvin-bot bot commented May 20, 2024

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

@melvin-bot melvin-bot bot removed the Overdue label May 20, 2024
@CortneyOfstad CortneyOfstad removed the Help Wanted Apply this label when an issue is open to proposals by contributors label May 20, 2024
@CortneyOfstad
Copy link
Contributor

@jjcoffee most recent update from @VickyStash is here — any questions, just let us know!

@jjcoffee
Copy link
Contributor

Okay looks like we're just waiting for @VickyStash to provide another update then?

@VickyStash
Copy link
Contributor

@jjcoffee Yeah, I'm spending the time today on this one and hope to provide updates later 🔍

@VickyStash
Copy link
Contributor

Updates:

I've noticed a slightly different behavior in how the navigation replace method in the Expensify app and the simple app I've prepared. The reason for this is that there is a patch to control some navigation methods in a custom way. The bug described in the issue relates specifically to this part:

+ // and remove any entries from history which focued route no longer exists in state
+ // That may happen if we replace a whole navigator.
+ const staleHistoryDiff = getStaleHistoryDiff(history.items.slice(0, history.index + 1), state);
+ if (staleHistoryDiff <= 0) {
+ history.replace({
+ path,
+ state
+ });
+ } else {
+ await history.go(-staleHistoryDiff);
+ history.push({
+ path,
+ state
+ });
+ }

I've made a minor update, and it seems to fix the problem, but I want to do more testing to see if there are any side effects.

navigation_fix1.mp4

@CortneyOfstad
Copy link
Contributor

Awesome — thanks @VickyStash!

@VickyStash
Copy link
Contributor

Updates:

Unfortunately, even with my approach, I have found a problem that the user can then move forward to other screens using browser buttons.

Simulator.Screen.Recording.-.iPhone.14.-.2024-05-23.at.18.51.56.mp4

I'm still looking for a safe way to update the browser history to make both - swipes and browser navigation works as expected.

@melvin-bot melvin-bot bot added the Overdue label May 23, 2024
Copy link

melvin-bot bot commented May 23, 2024

@CortneyOfstad @cristipaval @VickyStash @jjcoffee 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!

@jjcoffee
Copy link
Contributor

Chill Melv, this is under investigation 😄

@melvin-bot melvin-bot bot removed the Overdue label May 24, 2024
Copy link

melvin-bot bot commented May 27, 2024

@CortneyOfstad, @cristipaval, @VickyStash, @jjcoffee Whoops! This issue is 2 days overdue. Let's get this updated quick!

@melvin-bot melvin-bot bot added the Overdue label May 27, 2024
@VickyStash
Copy link
Contributor

Unfortunately, I still haven't found a safe way to eliminate this problem.

Hey @adamgrzybowski, it looks like you were working on handling browser history during complex navigation flows. Maybe you have any thoughts about it?

Context:
When the app dismisses a modal and navigates to the Report screen the replace method is called.
You've updated useLinking logic so the browser history handles the navigation in alignment with the app and user navigated to the right place using browser back/forward buttons.
But in the mweb safari if a user uses a swipe gesture to go back he sees previous screen during the swipe, but navigates to another right place.

@CortneyOfstad
Copy link
Contributor

Thanks @VickyStash!

@VickyStash
Copy link
Contributor

Me and @adamgrzybowski had a discussion yesterday, and unfortunately it looks like there is no safe way to get rid of the described issue without side effects.
Overall, it looks like it's more of an internal Safari feature to show a cached view of the previous route on swipe, and there are not many things we can do about it.
cc @jjcoffee

@jjcoffee
Copy link
Contributor

@VickyStash Thanks for looking into it! Just for completeness could you outline what the side effects are if you implement a fix for the issue?

@melvin-bot melvin-bot bot removed the Overdue label May 29, 2024
@VickyStash
Copy link
Contributor

@jjcoffee Any attempts to resolve the issue affected the browser history, leading to wrong navigation using the browser back/forward buttons on all of the web platforms.

@jjcoffee
Copy link
Contributor

Okay sounds like we should close this then, unless others disagree? @CortneyOfstad @cristipaval

@CortneyOfstad
Copy link
Contributor

Sounds good and thank you @VickyStash for your incredibly thorough investigation on this! Closing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
Development

No branches or pull requests

6 participants