[HOLD for payment 2023-09-07] [$1000] Chat - Opened chat does not show up in LHN when LHN is loading

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Issue found when executing PR #24204

Action Performed:

1. Go to staging.new.expensify.com
2. Log in with account with many conversations
3. Copy link to any chat
4. Log out
5. Navigate to the link in Step 1
6. Log in
7. Right after the chat is loaded, return to LHN

Expected Result:

The opened chat in Step 6 will appear in LHN while the rest of the chats is loading

Actual Result:

The opened chat in Step 6 does not appear in LHN even though it is opened prior

Workaround:

Unknown

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.54.7

Reproducible in staging?: Yes

Reproducible in production?: No

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

Notes/Photos/Videos: Any additional supporting documentation

Bug6165912_24204_mweb__1_.1.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~013d3358a3bd902de6
• Upwork Job ID: 1691926354859388928
• Last Price Increase: 2023-08-16
• Automatic offers:
• s-alves10 | Contributor | 26165190

[OSBotify]

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

[s-alves10]

Proposal

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

Opened chat doesn't show up in LHN when LHN is loading

What is the root cause of that problem?

Please check the below code

App/src/pages/home/sidebar/SidebarLinks.js

Lines 231 to 251 in 8472f03

	{this.props.isLoading ? (
	<>
	{lodashGet(this.props.report, 'reportID') && (
	<OptionRowLHNData
	reportID={this.props.currentReportID}
	viewMode={viewMode}
	shouldDisableFocusOptions={this.props.isSmallScreenWidth}
	onSelectRow={this.showReportPage}
	/>
	)}
	<OptionsListSkeletonView shouldAnimate />
	</>
	) : (
	<LHNOptionsList
	contentContainerStyles={[styles.sidebarListContainer, {paddingBottom: StyleUtils.getSafeAreaMargins(this.props.insets).marginBottom}]}
	data={this.props.optionListItems}
	onSelectRow={this.showReportPage}
	shouldDisableFocusOptions={this.props.isSmallScreenWidth}
	optionMode={viewMode}
	/>
	)}

As you can see, we show skeleton when props.isLoading is true, or show report list. Before this PR, props.isLoading is false when reports are not empty, so if any report was loaded, we don't show skeleton. We show preloaded report list while loading all reports. But now, isLoading is true and this.props.report is also empty because currentReportID is undefined. So skeleton is shown full page.

This is the root cause

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

When we open a report and navigate to the LHN, this.props.optionListItems is not empty. We should use this

Solution 1

Replacing the this.props.isLoading condition here with the this.props.isLoading && !this.props.optionListItems.length would give the exactly same result as production

Solution 2
I think it would be good to show existing reports with skeleton while loading. In order to do this, we can use the following code

        {this.props.isLoading ? (
            <>
                <LHNOptionsList
                    style={[styles.flexShrink1]}
                    contentContainerStyles={[styles.sidebarListContainer, {paddingBottom: StyleUtils.getSafeAreaMargins(this.props.insets).marginBottom}]}
                    data={this.props.optionListItems}
                    onSelectRow={this.showReportPage}
                    shouldDisableFocusOptions={this.props.isSmallScreenWidth}
                    optionMode={viewMode}
                />
                <OptionsListSkeletonView shouldAnimate />
            </>
        ) : (
            <LHNOptionsList
                contentContainerStyles={[styles.sidebarListContainer, {paddingBottom: StyleUtils.getSafeAreaMargins(this.props.insets).marginBottom}]}
                data={this.props.optionListItems}
                onSelectRow={this.showReportPage}
                shouldDisableFocusOptions={this.props.isSmallScreenWidth}
                optionMode={viewMode}
            />
        )}

Here we need to add additional props style to the LHNOptionList component. Its default value is styles.flex1 and this code

App/src/components/LHNOptionsList/LHNOptionsList.js

Line 70 in 8472f03

<View style={[styles.flex1]}>

will be replaced with

    style={props.style}

Solution 1

solution_1.mp4

Solution 2

solution_2.mp4

What alternative solutions did you explore? (Optional)

[melvin-bot]

Triggered auto assignment to @grgia (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[aldo-expensify]

cc @tienifr @ArekChr the PR #24204 seems to have caused this regression according to @s-alves10 , can you help confirm that?

Thanks @s-alves10 for the help investigating.

[ArekChr]

cc @tienifr @ArekChr the PR #24204 seems to have caused this regression according to @s-alves10 , can you help confirm that?

Thanks @s-alves10 for the help investigating.

@aldo-expensify Yes, this PR caused regression

[tienifr]

yes, it's my regression. I'll fix it asap

[yuwenmemon]

@tienifr any ETA on when you expect to have a PR up? If it's not going to be within the next few hours then should we revert the offending PR?

[s-alves10]

I can create a PR according to #24596 (comment) in 2~3 hours

[yuwenmemon]

@s-alves10 great - you're hired.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~013d3358a3bd902de6

[melvin-bot]

Triggered auto assignment to @sakluger (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[melvin-bot]

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

[s-alves10]

@tienifr

I think showing all existing reports is what the previous version does. You're trying to show only one report, but this isn't a solution anyway, I think. Let me check your case

[narefyev91]

@yuwenmemon @aldo-expensify We get in a situation where we have 2 open PR's to the same fix
PR from @tienifr already tested by @ArekChr and approved.
I can also start my part for PR from @s-alves10 - but do not want to have double work - and also based on Expected result "The opened chat in Step 6 does not appear in LHN even though it is opened prior" - i checked code from @tienifr and it fixes this issue.
And based on this comment #24596 (comment) - we will have 2 more issues - order problem and loading time
I will suggest to merge @tienifr PR and close for now @s-alves10 PR

[s-alves10]

@narefyev91

I was already assigned to this issue and created the PR. I'm not sure why my PR should be closed.
I think this kind of situations should be handled fairly.

PS: My solution behaves the same as the old version, but I guess @tienifr 's solution tries to show only one chats. This is not expected, I think.

[ArekChr]

@s-alves10 Here is the original issue #23735

[s-alves10]

The opened chat in Step 6 will appear in LHN while the rest of the chats is loading

This is the expected behavior of this issue. If we should avoid partial showing in wide-screen devices, we can add an additional condition !!this.props.currentReportID to the LHNOptionList.

I don't think this can be a reason for closing the PR
Expecting others' thoughts here

[s-alves10]

@yuwenmemon @aldo-expensify @narefyev91

Please share your thoughts here. What should I do?

[s-alves10]

Showing one report causes this regression as well

[aldo-expensify]

Sorry, for the delay, can we summarize what each PR is fixing and the pros and cons?

My current understanding:

@tienifr 's PR #24619 is fixing the issue reported here, right? I personally don't like much the solution using the state firstLoadedReportID... it feels like a workaround, but that doesn't mean it is not acceptable.

@s-alves10 's PR #25159 feels better to me from a code perspective, and is supposed to be fixing this issue and also the other regression. The problem is that this seem to cause some unwanted effects regarding the order of the reports in the LHN, did I get that correctly?

Would be good to know:

• if @tienifr 's PR fix: regression 23735 #24619 also fixes other regression or not?
• What are exactly the bad side effects that @s-alves10 's PR fix: show existing reports while loading #25159 causes and if they can be improved. Is the ordering problem caused because we render too early the LHN? is it what we see at the end of the video posted here?
• Is there any bad side effect caused by @tienifr 's PR fix: regression 23735 #24619 ?

[aldo-expensify]

I know I said I would come back to this later today, but I got caught in something I though would be simpler. I'll check in tomorrow!

[s-alves10]

@aldo-expensify @narefyev91

I've added some updates to the PR to fix the issue mentioned here.

Please check

[narefyev91]

@aldo-expensify after checking both PR's i think @s-alves10 PR looks preferable - we will not use state, and all logic will be inside optionListItems - which makes more sense. BTW let us know next steps here

[aldo-expensify]

Agreed, I tested and seems to be solving both issues (commented with more details here).

Let's wait for @s-alves10 to update the PR with main and resolve conflicts, then we can do a final review.

[melvin-bot]

Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:

• when @s-alves10 got assigned: 2023-08-16 21:35:15 Z
• when the PR got merged: 2023-08-29 17:56:11 UTC
• days elapsed: 8

On to the next one 🚀

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.59-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• fix: show existing reports while loading #25159

If no regressions arise, payment will be issued on 2023-09-07. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

• @narefyev91 does not require payment (Contractor)
• @s-alves10 requires payment offer (Contributor)

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[sakluger]

Summarizing payouts for this issue:

Reporter: Applause (contractor, no payment)
Contributor: @s-alves10 $1000 (hired on Upwork)
Contributor+: @narefyev91 (contractor, no payment)

Above payments include efficiency bonus 🎉
Upwork job: https://www.upwork.com/jobs/~0137e6674165abf18a