[$500] Thread - Main chat is in a different section when returning from thread via browser back button

[lanitochka17]

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.46
Reproducible in staging?: Y
Reproducible in production?: Y
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. Navigate to staging.new.expensify.com
2. Log in.
3. Go to the public room https://staging.new.expensify.com/r/6776919265133947
4. Scroll to the top to reveal all the conversation.
5. Scroll down and open any thread (better visit newer thread).
6. Click browser back button.

Expected Result:

App will return to the same part of the main chat as before going to the thread

Actual Result:

App will return to the same part of the main chat as before going to the thread

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

Bug6385116_1708383407589.20240220_013812.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0123cd735ed53880ac
• Upwork Job ID: 1765499258797154304
• Last Price Increase: 2024-03-06

[melvin-bot]

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

[lanitochka17]

@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

[lanitochka17]

We think that this bug might be related to #wave5
CC @dylanexpensify

[slafortune]

@CortneyOfstad is OOO until 3/11 - reassigning

[melvin-bot]

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

[joekaufmanexpensify]

Reproduced. This affects any chat, not just public rooms. @hayata-suenaga I see you worked on this. I feel like this could be related to ideal nav. Curious if you agree?

[hayata-suenaga]

@joekaufmanexpensify, thank you for informing us about this issue. It appears to be related to the preservation of chat list position, rather than the back navigation behavior.

Just in case, I'm tagging @getusha and @adamgrzybowski from the linked issue.

[jacobkim9881]

Hi. I found a way round that doesn't make the bug.

2024-03-06.6.11.34.mov

[jacobkim9881]

I tried to find how two actions(clicking thread and clicking forward button) different. I found clicking the thread makes this log.

The log before clicking any thread, which made the bug(OpenReport command is executed)


[Debug] [info] Called API write - {"command":"OpenReport","reportID":"6962315901589981","emailList":"","accountIDList":"","parentReportActionID":"0","idempotencyKey":"OpenReport_6962315901589981","clientLastReadTime":"2024-03-06 08:01:35.422"}

It seems OpenReport cause the bug. I wanted to know how the OpenReport works. I found this comments on the code.

// If you already have a report open and are deeplinking to a new report on native, // the ReportScreen never actually unmounts and the reportID in the route also doesn't change. // Therefore, we need to compare if the existing reportID is the same as the one in the route // before deciding that we shouldn't call OpenReport.

It is when any thread clicked. That means clicking threads urges ReportScreen to unmount current reportID to mount new reportID for the thread.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0123cd735ed53880ac

[melvin-bot]

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

[joekaufmanexpensify]

Looking for proposals

[ahmedGaber93]

Proposal

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

Thread - Main chat is in a different section when returning from thread via browser back button

What is the root cause of that problem?

when we back to the main thread and the component MVCPFlatList is not complete layout the firstVisibleViewOffset will calculated wrong because firstVisibleView.offsetTop be 0 here until the component complete layout.

App/src/components/FlatList/MVCPFlatList.js

Line 95 in ffa731a

const firstVisibleViewOffset = horizontal ? firstVisibleView.offsetLeft : firstVisibleView.offsetTop;

and after the component complete layout onLayout we missed calculatefirstVisibleViewOffset again.

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

we need to call prepareForMaintainVisibleContentPosition() inside onLayout to calaculate firstVisibleViewOffset correctly

What alternative solutions did you explore? (Optional)

[tienifr]

Proposal

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

App will return to the same part of the main chat as before going to the thread

What is the root cause of that problem?

We have the maintaining visible content position logic here that will look at the delta between the firstVisibleViewOffset and its previous value (prevFirstVisibleOffset) to add the proper scroll to maintain the visible content position that the user is looking at.

Let's say currently the firstVisibleView.offsetTop is 500.

When we navigate to the thread, the report action list scroll view will become invisible and its scrollHeight will be 0. When a scroll view's scrollHeight is 0, of course the firstVisibleView.offsetTop will also be 0 (the offset of an item cannot be higher than the scroll view's scroll height). Thus the prevFirstVisibleOffsetRef.current will be set to 0 here.

Then when we come back to the parent report, the scroll view now becomes visible, and the firstVisibleView.offsetTop will be 500 again. However since the prevFirstVisibleOffsetRef is set to 0 incorrectly earlier, there's still a delta between the current and previous values, thus leading to the scroll view being scrolled.

This has nothing to do with onLayout, just that the prevFirstVisibleOffsetRef is being set incorrectly to 0. The logic in adjustForMaintainVisibleContentPosition is supposed to "correct" the position in case the view hierarchy of the current scroll view changes.

To quote from here

The general idea is to get the position of the first visible view before changes to the view hierarchy, compare it to its position after the changes and adjust the scroll position accordingly.

It's not supposed to run in case the offset of the first visible item changes to 0 due to the scroll view becomes invisible.

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

We can simply not run the adjustForMaintainVisibleContentPosition if the scroll view becomes hidden (it will have scrollHeight 0 in this case).

In here

const contentView = getContentView();
if (contentView && contentView.scrollHeight === 0) {
    return;
}

(contentView is the scroll view, also used in other places in the MVCPFlatList)

This will fix the root cause and no redundant additional calculation needed.

What alternative solutions did you explore? (Optional)

NA

[jacobkim9881]

Seems it will be fixed here.

[joekaufmanexpensify]

Not overdue

[joekaufmanexpensify]

@mkhutornyi curious if you agree this will be fixed in this PR?

[jacobkim9881]

Let me clear this. I tested it with this PR and checked the issue is solved.

[joekaufmanexpensify]

Sounds good. Closing this then since this issue will be fixed there!