App
App copied to clipboard
Expensify
[$500] Report - The conversation scrolls to the beginning when you open the settings menu
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] 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:
- Open https://staging.new.expensify.com/
- Log in under your HT account A
- Navigate to a conversation with more messages
- Scroll to the top of your conversation history so that the last message is not visible
- Open settings via avatar
- 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
Upwork Automation - Do Not Edit
- Upwork Job URL: https://www.upwork.com/jobs/~0154928522b8004b1d
- Upwork Job ID: 1753894702380920832
- Last Price Increase: 2024-02-03
Job added to Upwork: https://www.upwork.com/jobs/~0154928522b8004b1d
![melvin-bot[bot] avatar](https://avatars.githubusercontent.com/in/179547?v=4 "Published by a pub.dev verified publisher"){kind=small}
Triggered auto assignment to Contributor-plus team member for initial proposal review - @akinwale (External)
Triggered auto assignment to @anmurali (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
: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:
- Identify the pull request that introduced this issue and revert it.
- Find someone who can quickly fix the issue.
- Fix the issue yourself.
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
Triggered auto assignment to @neil-marcellini (Engineering), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.
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)
When I press back, I'm taken to LHN, not the previous screen as OP has mentioned.
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
@sahilnagpal26's proposal correctly identifies the root cause and the suggested solution is valid.
🎀👀🎀 C+ reviewed.
Current assignee @neil-marcellini is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.
@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.
@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).
@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 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.
📣 @akinwale 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!
📣 @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 📖
Hey @hayata-suenaga isn't that a duplicate? https://github.com/Expensify/App/issues/35602 It looks similar to me
Looks similar but accepted solution here doesn't fix https://github.com/Expensify/App/issues/35602
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
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.
Triggered auto assignment to @anmurali (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
Triggered auto assignment to @trjExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
Rotated the label since I will be OOO till 29th! I can take it back when I return! Thank you @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.
⚠️ 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.
⚠️ 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.