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

Status - Inconsistent behavior when closing status RHP #35729

Closed
4 of 6 tasks
kbecciv opened this issue Feb 2, 2024 · 10 comments
Closed
4 of 6 tasks

Status - Inconsistent behavior when closing status RHP #35729

kbecciv opened this issue Feb 2, 2024 · 10 comments
Assignees
Labels
Engineering Monthly KSv2 Reviewing Has a PR in review

Comments

@kbecciv
Copy link

kbecciv commented Feb 2, 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: v.1.4-36.0
Reproducible in staging?: y
Reproducible in production?: n
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 has a custom status.

  1. Go to staging.new.expensify.com.
  2. On home page, click on status icon on the user avatar.
  3. Click outside the status RHP.
  4. Return to main chat.
  5. Click on status icon.
  6. Click back button on the RHP.

Expected Result:

There should be consistent behavior when closing status RHP when clicking outside the RHP and clicking back button on RHP.

Actual Result:

In Step 6, when clicking back button on the status RHP, it closes both profile page and also status RHP.
On Android app, the back button on status page redirects user to the home page, while the device back navigation on status page reveals the profile page underneath.

Workaround:

Unknown

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

Bug6365303_1706906464990.20240203_041534.mp4

View all open jobs on GitHub

@kbecciv kbecciv added the DeployBlockerCash This issue or pull request should block deployment label Feb 2, 2024
Copy link
Contributor

github-actions bot commented Feb 2, 2024

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

Copy link

melvin-bot bot commented Feb 2, 2024

Triggered auto assignment to @roryabraham (Engineering), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

@kbecciv
Copy link
Author

kbecciv commented Feb 2, 2024

We think that this bug might be related to #vip-vsb
CC @quinthar

@bernhardoj
Copy link
Contributor

bernhardoj commented Feb 3, 2024

Proposal

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

Pressing the status page back button takes the user back to the report screen.

What is the root cause of that problem?

If we press the overlay, it simply calls goBack which will close the status page.

<Overlay
onPress={() => {
if (isExecutingRef.current) {
return;
}
isExecutingRef.current = true;
navigation.goBack();

But if we press the status page back button, it will pop all screens (shouldPopToTop is true)

const navigateBackToPreviousScreen = useCallback(() => Navigation.goBack('', false, true), []);

if (shouldPopToTop) {
if (shouldPopAllStateOnUP) {
shouldPopAllStateOnUP = false;
navigationRef.current?.dispatch(StackActions.popToTop());
return;
}
}

and only happens if we press the status icon.

Navigation.setShouldPopAllStateOnUP();
Navigation.navigate(ROUTES.SETTINGS_STATUS);

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

I think the pop to top logic is not relevant anymore for status page after the navigation change because the setting page always open, so we can update the go back code in status page to simply call goBack.

What alternative solutions did you explore? (Optional)

Call setShouldPopAllStateOnUP only for small screen, but it would be weird if we resize from small to large screen.

Or we can keep calling setShouldPopAllStateOnUP and pass shouldPopToTop to goBack only if it's a small screen.

Navigation.goBack('', false, isSmallScreenWidth)

@melvin-bot melvin-bot bot added the Overdue label Feb 5, 2024
@mountiny mountiny added Daily KSv2 and removed DeployBlockerCash This issue or pull request should block deployment Hourly KSv2 labels Feb 5, 2024
@melvin-bot melvin-bot bot removed the Overdue label Feb 5, 2024
@mountiny
Copy link
Contributor

mountiny commented Feb 5, 2024

Demoting this issue from deploy blocker, this was kinda known before the merge and related to goBack functionality.

@adamgrzybowski Could you please look into the proposal from @bernhardoj to see whether we should go with it or wait for the more generic goBack fix ? Thanks!

@hayata-suenaga
Copy link
Contributor

hayata-suenaga commented Feb 5, 2024

All back issues are tracked in this issue, and I have added this particular issue to it as well. 👍

@mountiny
Copy link
Contributor

mountiny commented Feb 8, 2024

Making this weekly given its part of a bigger scheme

@adamgrzybowski
Copy link
Contributor

Probably mentioned somewhere but it should be fixed by this PR

@mountiny mountiny added the Reviewing Has a PR in review label Feb 9, 2024
@melvin-bot melvin-bot bot removed the Weekly KSv2 label Mar 4, 2024
Copy link

melvin-bot bot commented Mar 4, 2024

This issue has not been updated in over 15 days. @mountiny, @hayata-suenaga eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

@melvin-bot melvin-bot bot added the Monthly KSv2 label Mar 4, 2024
@hayata-suenaga
Copy link
Contributor

Probably mentioned somewhere but it should be fixed by this #36050

Screen.Recording.2024-03-04.at.11.05.56.AM.mov

confirmed the issue was fixed on production. closing the issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering Monthly KSv2 Reviewing Has a PR in review
Projects
No open projects
Development

No branches or pull requests

6 participants