[Awaiting Payment March 20th] [$500] Report - The conversation scrolls to the beginning when you open the settings menu

[izarutskaya]

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.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:

1. Open https://staging.new.expensify.com/
2. Log in under your HT account A
3. Navigate to a conversation with more messages
4. Scroll to the top of your conversation history so that the last message is not visible
5. Open settings via avatar
6. Close the setting via the "Back" arrow

Expected Result:

When you close the settings menu, the conversation should not restart and scroll back to the beginning.

Actual Result:

The conversation scrolls to the beginning when you open the settings menu

Workaround:

Unknown

Platforms:

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

• [ ] Android: Native
• [ ] Android: mWeb Chrome
• [ ] iOS: Native
• [ ] iOS: mWeb Safari
• [x] MacOS: Chrome / Safari
• [x] MacOS: Desktop

Screenshots/Videos

https://github.com/Expensify/App/assets/115492554/3f6eb2b3-3f74-4d7b-b6e5-278cd8e79c66

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0154928522b8004b1d
• Upwork Job ID: 1753894702380920832
• Last Price Increase: 2024-03-18
• Automatic offers:
• akinwale | Reviewer | 0

Issue Owner

Current Issue Owner: @trjExpensify

[melvin-bot[bot]]

Job added to Upwork: https://www.upwork.com/jobs/~0154928522b8004b1d

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[github-actions[bot]]

:wave: 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.

[melvin-bot[bot]]

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

[izarutskaya]

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

[sahilnagpal26]

Proposal

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

The conversation scrolls to the beginning when you open the settings menu

What is the root cause of that problem?

This happens because when we press the back button, we call closeFullScreen which leads to this state https://github.com/Expensify/App/blob/3ab4e6e12d9ff38e69493321f0d31f032481b373/src/pages/settings/InitialSettingsPage.js#L347

closeFullScreen uses StackActions.popToTop()which takes us back to the first screen in the stack, dismissing all the others. Once this happens, ReportScreen Component is rendered, however, the report prop in this case is set to {}. ReportScreen component renders ReportActionsView which internally invokes openReportIfNecessary. As props.report.isOptimisticReport is false, Report.openReport(reportID) is invoked, leading to a network call and fetching the messages again. This leads to re-rendering of the messages and the movement to the most recent message in the chat.

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

We can instead use Navaigation.dismissModal on the press of the back button. This internally invokes StackActions.pop(). In this case when ReportScreen component is rendered, we already have the report prop. So, when ReportActionsView which internally invokes openReportIfNecessary. As props.report.isOptimisticReport is true, it simply returns without invoking Report.openReport(reportID) and hence the scroll is maintained.

https://github.com/Expensify/App/blob/3ab4e6e12d9ff38e69493321f0d31f032481b373/src/libs/Navigation/Navigation.ts#L54-L60

What alternative solutions did you explore? (Optional)

[muas19]

When I press back, I'm taken to LHN, not the previous screen as OP has mentioned.

[mountiny]

This does not need to be a blocker, user is not limited in use of the app

@akinwale please review the proposals once you will have time, thank you

[akinwale]

@sahilnagpal26's proposal correctly identifies the root cause and the suggested solution is valid.

🎀👀🎀 C+ reviewed.

[melvin-bot[bot]]

Current assignee @neil-marcellini is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[sahilnagpal26]

@akinwale As this is going to be my first contribution. I wanted to understand what should be my next action here? Should I create a PR For the same.

[akinwale]

@sahilnagpal26 You will be assigned to the issue, but you can go ahead and create the PR so that you can get all the first-timer tasks done (signing the CLA, etc).

[neil-marcellini]

@sahilnagpal26's proposal correctly identifies the root cause and the suggested solution is valid.

🎀👀🎀 C+ reviewed.

@sahilnagpal26 It seems like the solution probably works given that it's easy to test, but does it have unwanted side effects? I think the root cause analysis needs a lot more explanation. Why is Navigation.closeFullScreen() not the right approach? Why is it causing the report to scroll back to the latest message?

I want to make sure I understand this, given that I don't have much experience with our navigation system. Please edit your proposal and let me know when it's updated. Please refrain from creating a PR until you are assigned by an internal engineer.

[sahilnagpal26]

Proposal

Updated

[neil-marcellini]

I will review the updated proposal today

[situchan]

@adamgrzybowski can confirm if solution is fine

[neil-marcellini]

@sahilnagpal26 thank you for updating your proposal with a better root cause explanation. It makes sense to me. However, I don't think your explanation of the solution is correct, regarding why openReport is not called.

This internally invokes StackActions.pop(). In this case when ReportScreen component is rendered, we already have the report prop. So, when ReportActionsView which internally invokes openReportIfNecessary. As props.report.isOptimisticReport is true, it simply returns without invoking Report.openReport(reportID) and hence the scroll is maintained.

It doesn't make sense that the report is optimistic. I think StackActions.pop() is popping the settings page, without popping/unmounting the report screen behind it, so the ReportScreen does not call openReportIfNecessary when it re-mounts here. https://github.com/Expensify/App/blob/04795f9915e39edda75f7613030c7a3bf4e5cfed/src/pages/home/report/ReportActionsView.js#L115-L118

Maybe @adamgrzybowski can provide a more in depth explanation, but for now I'm happy with the current explanation and you can start on the PR.

[melvin-bot[bot]]

📣 @akinwale 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

[melvin-bot[bot]]

📣 @sahilnagpal26 You have been assigned to this job! Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs! Keep in mind: Code of Conduct | Contributing 📖

[adamgrzybowski]

Hey @hayata-suenaga isn't that a duplicate? https://github.com/Expensify/App/issues/35602 It looks similar to me

[situchan]

Looks similar but accepted solution here doesn't fix https://github.com/Expensify/App/issues/35602

[adamgrzybowski]

The dismissModal should be ok in that case.

But there is one related problem. For some reason pop() is much slower than the popToTop().

I agree that we want to use pop() but not sure if we want to switch to it before fixing the performance problem. I'm currently looking into this

If I remember correctly @mountiny noticed the performance problem when we were working on the draft PR for ideal nav. Maybe he have some thought on this topic

[hayata-suenaga]

Hey @hayata-suenaga isn't that a duplicate? https://github.com/Expensify/App/issues/35602 It looks similar to me

The referenced issue pertains to the position of the chat list on the Left Hand Navigation (LHN), not the list of chat messages. While the issues may look similar, the solution for the linked issue does not resolve the problem described here.

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[anmurali]

Rotated the label since I will be OOO till 29th! I can take it back when I return! Thank you @trjExpensify

[trjExpensify]

Lucky, lucky me! :) All good, looks like we're waiting for the PR to hit prod so we can start the clock on payment.

[melvin-bot[bot]]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[melvin-bot[bot]]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[situchan]

This caused multiple regressions above ^

[sahilnagpal26]

Hey team, what is exactly happening here? Is the solution breaking something?

[trjExpensify]

yep, this PR was reverted as you can see here: https://github.com/Expensify/App/pull/36936

In that PR there are two issues that were flagged as deploy blockers as a result of the PR. So those need to be fixed in order to proceed with a new PR.

[trjExpensify]

@sahilnagpal26 @akinwale what are the next steps here, and who's progressing them? Thanks!

[trjExpensify]

Removing the Reviewing label as the initial PR for this was reverted.

[melvin-bot[bot]]

@akinwale, @anmurali, @trjExpensify, @neil-marcellini, @sahilnagpal26 Whoops! This issue is 2 days overdue. Let's get this updated quick!

[neil-marcellini]

bump @sahilnagpal26. Would you please post a new proposal including the root cause of the regressions and an update solution?

[melvin-bot[bot]]

@akinwale @anmurali @trjExpensify @neil-marcellini @sahilnagpal26 this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

• Decide whether any proposals currently meet our guidelines and can be approved as-is today
• If no proposals meet that standard, please take this issue internal and treat it as one of your highest priorities
• If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[trjExpensify]

@sahilnagpal26 - can you confirm you'll be addressing the follow-up required here to fix the regressions and redeploy? Thanks!

[melvin-bot[bot]]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[sahilnagpal26]

@trjExpensify I will be working on this. Let me submit the proposal again for the same.

[trjExpensify]

Please do, let's keep the urgency on this!

[trjExpensify]

@sahilnagpal26 can we get an update and ETA from you, please?

[melvin-bot[bot]]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[sahilnagpal26]

@trjExpensify I am trying to figure out a solution, however, its taking time longer than I expected.

[trjExpensify]

Appreciate the update. If you need to brainstorm, please do feel encouraged to use the #expensify-open-source channel and tag a few people that have been involved in the original PR(s).

[adamgrzybowski]

@sahilnagpal26 Make sure that you have the newest changes. There were some changes related to the navigation in this PR

[trjExpensify]

@sahilnagpal26 do you have a plan to address the issues this week?