[HOLD for payment 2024-02-07] [$500] Search - Focused account hidden by "Star a chat, get $250." when navigating with an arrow

[lanitochka17]

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.22-0
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. Login to NEwDot
2. Click on the green + button
3. Click on the "Start chat" option
4. Go down the list around 10-15 accounts using keyboard arrows

Expected Result:

Focused account should be visible

Actual Result:

Focused account is hidden by "Star a chat, get $250." banner when navigating with an arrow

Workaround:

Unknown

Platforms:

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

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• Windows: Chrome
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6335899_1704738005020.video_68.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~010450be8409118daf
• Upwork Job ID: 1744446534096453632
• Last Price Increase: 2024-01-15
• Automatic offers:
• c3024 | Reviewer | 28120212
• bernhardoj | Contributor | 28120213

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~010450be8409118daf

[melvin-bot]

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

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[melvin-bot]

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

[ikevin127]

Proposal

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

Focused account is hidden by referral "Star a chat, get $250." banner when navigating with Up / Down arrows.

What is the root cause of that problem?

This happens because when using navigating with Up / Down arrows the scrollToIndex() function from BaseOptionsSelector.js does not account for the extra spacing added by the introduction of the ReferralCTA at the bottom of the ScrollView and even though we focused the item, the focused item is hidden behind the referral element.

This can be easily reproduced by navigating from first to last item by pressing Up arrow once.

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

A solution for this case is to add negative viewOffset to the SectionList's scrollToLocation function which calls the scroll to the focused item. We only add negative offset for items with index > 0 so that the first item won't be affected by the offset and hidden.

Videos

MacOS: Chrome / Safari

Screen.Recording.2024-01-08.at.23.53.26.mov

[jayeshmangwani]

I am not able to reproduce this issue on mac chrome, @ikevin127 are you able to reproduce the issue ?

[ikevin127]

@jayeshmangwani I'm on MacOS too and I can repro on Chrome, Firefox and Safari.

It can be easily reproduced by navigating from first to last item by pressing Up arrow once.

[tienifr]

Proposal

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

Focused account is hidden by "Star a chat, get $250." banner when navigating with an arrow

What is the root cause of that problem?

We're putting empty sections into SectionList inside OptionsSelector in NewChatPage, this causes the native SectionList to calculate scrolling distance incorrectly, leading to this issue. Meanwhile, other pages that also use BaseOptionsSelector but doesn't pass in empty sections, do not have this issue, for example the search page.

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

Filter out the empty sections before further processing, we can do this either before passing the sections into OptionsSelector (like NewChatPage), or inside BaseOptionsSelector itself (filtering the props.sections passed in)

This empty sections will not show in the UI at all and is safe to be removed. Then we can also remove the workarounds that we had to add because of the empty section like here

We need to also check other usage of BaseOptionsSelector in other pages and apply the same fix, and potentially double check the BaseSelectionList as well.

What alternative solutions did you explore? (Optional)

NA

[bernhardoj]

Proposal

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

When scrolling through the new chat list with a keyboard arrow, the bottom items are hidden by the referral section.

What is the root cause of that problem?

In short: we scroll to the wrong index

Detail:
When we use the arrow key, it will call scrollToIndex for each item. We need a section and item index to scroll inside a SectionList. If we look at the scroll logic, we are reducing the section index for every empty section (section without data)

App/src/components/OptionsSelector/BaseOptionsSelector.js

Lines 394 to 404 in d1e1550

	// Note: react-native's SectionList automatically strips out any empty sections.
	// So we need to reduce the sectionIndex to remove any empty sections in front of the one we're trying to scroll to.
	// Otherwise, it will cause an index-out-of-bounds error and crash the app.
	let adjustedSectionIndex = sectionIndex;
	for (let i = 0; i < sectionIndex; i++) {
	if (_.isEmpty(lodashGet(this.state.sections, `[${i}].data`))) {
	adjustedSectionIndex--;
	}
	}
	this.list.scrollToLocation({sectionIndex: adjustedSectionIndex, itemIndex, animated});

And the reason for that is written in the comment above. But if we log inside the getItemLayout (the data, 1st param), we can see that the empty sections are still available on the list data, not stripped out.

On the new chat page, we have one empty section (index 0) for selected users (if we haven't select anyone),

App/src/pages/NewChatPage.js

Lines 78 to 80 in 0c5a2cb

	const formatResults = OptionsListUtils.formatSectionsFromSearchTerm(searchTerm, selectedOptions, filteredRecentReports, filteredPersonalDetails, {}, false, indexOffset);
	sectionsList.push(formatResults.section);
	indexOffset = formatResults.newIndexOffset;

and then we have the Recents (index 1) and Contacts (index 2) section.

So, every time we call scrollToIndex, it will scroll to the previous section location, for example, when we are navigating through the Contacts section with an arrow key, instead of scrolling to section 2, we scroll to section 1 because of the assumption that empty section is strips out from the list. That's why the scrolling location is wrong.

(we can verify this root cause by selecting at least one user (to group) and the scrolling will work fine)

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

The reducing section index logic was added ~ 2 years ago, maybe the bug happened at that time, but looks like it's not relevant anymore with the proof of getItemLayout is still called for empty section. So, we can remove the reducing section index logic.

App/src/components/OptionsSelector/BaseOptionsSelector.js

Lines 397 to 402 in d1e1550

	let adjustedSectionIndex = sectionIndex;
	for (let i = 0; i < sectionIndex; i++) {
	if (_.isEmpty(lodashGet(this.state.sections, `[${i}].data`))) {
	adjustedSectionIndex--;
	}
	}

I have tested on all platforms and no error happens.

[zanyrenney]

@c3024 please review the proposals.

[zanyrenney]

bump @c3024 please review the proposals? If you do not have capacity to work on this issue, please let me know and I can reassign 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? 💸

[c3024]

Reviewing.

[c3024]

Thanks for all your proposals.

I think compensating for the referral CTA is a workaround.

As the SectionList is not stripping away the empty sections, I think the simpler solution is to remove the code adjusting the index for scrolling.

So, @bernhardoj 's proposal here looks good to me.

🎀 👀 🎀 C+ Reviewed

[melvin-bot]

Triggered auto assignment to @techievivek, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[melvin-bot]

@techievivek @zanyrenney @c3024 this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[zanyrenney]

this shouldn't be flagging as overdue, I have just bumped the eng team.

[zanyrenney]

https://expensify.slack.com/archives/C01GTK53T8Q/p1706024665251019

[dangrous]

@c3024 your analysis seems correct, I think @bernhardoj has the simplest solution here and we can move forward!

[melvin-bot]

📣 @c3024 🎉 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]

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

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[bernhardoj]

PR is ready

cc: @c3024

[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.4.33-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix highlighted user in new chat page is covered by referral view #35129

If no regressions arise, payment will be issued on 2024-02-07. 🎊

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

• @bernhardoj requires payment automatic offer (Contributor)
• @c3024 requires payment automatic offer (Reviewer)

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@c3024] The PR that introduced the bug has been identified. Link to the PR:
• [@c3024] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@c3024] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@c3024] Determine if we should create a regression test for this bug.
• [@c3024] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@zanyrenney] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[c3024]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@c3024] The PR that introduced the bug has been identified. Link to the PR: Standardize arrow navigation in OptionsLists #7702
• [@c3024] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: https://github.com/Expensify/App/pull/7702/files#r1477340685
• [@c3024] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: No discussion has been started because there appears nothing that can be improved in the PR checklist that could have identified this earlier.
• [@c3024] Determine if we should create a regression test for this bug. Yes
• [@c3024] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Regression Test Proposal

The QA steps specified here is a suitable regression test

1. Open new chat page
2. Scroll down the list with the keyboard arrow down > 10 times
3. Verify the list keeps scrolling to the bottom and that the focused user/option is not covered by the referral view (should be around the middle)

👍 or 👎

[zanyrenney]

payment summary:

@bernhardoj requires payment automatic offer (Contributor) PAID $500
@c3024 requires payment automatic offer (Reviewer) PAID $500