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

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
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Bug6366093_1706977070977.Recording__1272.mp4

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]

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

[melvin-bot]

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

[melvin-bot]

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

[github-actions]

👋 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]

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

App/src/pages/settings/InitialSettingsPage.js

Line 347 in 3ab4e6e

onBackButtonPress={() => Navigation.closeFullScreen()}

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.

App/src/libs/Navigation/Navigation.ts

Lines 54 to 60 in 3ab4e6e

	const dismissModal = (reportID?: string, ref = navigationRef) => {
	if (!reportID) {
	originalDismissModal(ref);
	return;
	}
	const report = getReport(reportID);
	originalDismissModalWithReport({reportID, ...report}, ref);

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]

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.

[melvin-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?

[adamgrzybowski]

@trjExpensify Something to check, but I think this issue may no longer exist after the release of ideal-nav-v2. At least I can't reproduce it.

[melvin-bot]

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

[trjExpensify]

Oh, good point! Because we've moved the settings page to the avatar in the bottom tab, we don't push a "full screen" modal on top of the chats list anymore with the recent changes, right? So then I think the question evolves to:

When you switch between the bottom tabs, should your scroll position in the chat be maintained? It doesn't look like that's the case on staging, but I think you would want that.

dKHthPzh3y.mp4

Equally, I think you want your scroll position to be maintained when switching between two chats though, but it also seems like that's not working either:

LFd6EIFfP0.mp4

@roryabraham @mountiny @luacmartins @JmillsExpensify, am I missing something (maybe comment linking related?) as to why maintaining the scroll position inside the chat might not be working? 🤔

[adamgrzybowski]

@trjExpensify When we switch chats -> settings -> chats the second time we see a chat screen it is a different component than the first time we saw it. Even if it is the same report.

This is done to achieve the desired history pattern in the browser.

The disadvantage of this solution is that because the scroll position is saved on the screen, it is reset after such navigation

If you would go back twice in the browser after that you would see that the initial scroll position is still there.

To solve similar problem for the BottomTabNavigator I created this PR
It use global context to have consistent scroll position between different screen components with the same params.

I think this solution potentially could be extended to cover the report screen cases.

[trjExpensify]

Ah okay cool, so we have an issue for that PR to progress it. Would be good if it's extendable to cover the report screen cases, but we can look into that over there.

Okay, so I think we can move to close this issue out now. We do need to settle up with @sahilnagpal26 & @akinwale for the initial work prior to the app changes.

Payment summary as follows:

$250 to @sahilnagpal26 for the fix
$250 to @akinwale for the C+ review

Two regressions (1, 2) came out of the PR, but one was known, so deducted accordingly for only one per the contributor guidelines.

@akinwale, let me know I didn't miss anything on the above, and I'll go ahead and issue payments. 👍

[akinwale]

@trjExpensify #36888 was already a known issue prior to the PR being submitted as the performance problem was identified beforehand (context), so I think one regression deduction applies here.

[adamgrzybowski]

@trjExpensify

Ah okay cool, so we have an issue for that PR to progress it. Would be good if it's extendable to cover the report screen cases, but we can look into that over there.

I would merge the PR for scroll on the HOME page and ask somebody from SWM to use this code to create similar solution for the report screen scroll.

Do you want to create and issue for that?

[mountiny]

Seems like this got handled well. We got an issue for the bottom tabs and PR to fix it going on

@trjExpensify with the chats I guess it depends, you can see Slack keeps the position. WhatsApp desktop does not though, I think we can explore how to solve this but we need to make sure it will solve well and would not hurt performance even if we would store the position for many reports (or put some limit on 10 latest visited reports for example.)

[trjExpensify]

@trjExpensify #36888 was already a known issue prior to the PR being submitted as the performance problem was identified beforehand (#35763 (comment)), so I think one regression deduction applies here.

Ah okay, got it. Cool, updated the payment summary. Offers sent to you both!

[akinwale]

@trjExpensify Accepted. Thanks!

[trjExpensify]

@trjExpensify with the chats I guess it depends, you can see Slack keeps the position. WhatsApp desktop does not though, I think we can explore how to solve this but we need to make sure it will solve well and would not hurt performance even if we would store the position for many reports (or put some limit on 10 latest visited reports for example.)

Yeah, maybe we can bring this out for a discussion in somewhere like #vip-vsb to decide what we want to do. I agree we don't want to take a performance hit, but I do think keeping the position will be desirable when we use this a lot for all of our business chat purposes where you're switching a lot.

[trjExpensify]

@trjExpensify Accepted. Thanks!

Paid!

[trjExpensify]

@sahilnagpal26 waiting for you to accept the offer.

[trjExpensify]

@sahilnagpal26 paid. Closing!