[HOLD for payment 2024-03-07] [$500] IOU - "To" destination is highlighted in confirmation page

[kbecciv]

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: v1.4.44-0
Reproducible in staging?: y
Reproducible in production?: n
Issue reported by: Applause - Internal Team

Action Performed:

1. Go to staging.new.expensify.com
2. Go to FAB > Request money > Manual.
3. Enter amount > Next.
4. Select participant > Next.

Expected Result:

"To" destination will not be highlighted in confirmation page.

Actual Result:

"To" destination is highlighted in confirmation page.

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

Bug6393384_1708985967723.bandicam_2024-02-27_05-41-31-026.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0177dd8b03ddc70310
• Upwork Job ID: 1762248762916761600
• Last Price Increase: 2024-02-26
• Automatic offers:
• akinwale | Reviewer | 0
• paultsimura | Contributor | 0

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0177dd8b03ddc70310

[melvin-bot]

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

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

[kbecciv]

We think that this bug might be related to #vip-bills
CC @davidcardoza

[neil-marcellini]

👀 looking

[Tony-MK]

I think was caused by #35594.

Something must have been left out.

[neil-marcellini]

@Tony-MK why do you think it was caused by that PR?

[paultsimura]

Just as a side-effect, pressing Enter no longer proceeds to the request creation, but "clicks" the highlighted workspace option.

[Tony-MK]

My Apolgies 🙏.

After checking the PR videos, I don't believe the PR caused this.

[neil-marcellini]

The OptionRow has optionIsFocused set to true, setting the background color. That prop ultimately comes from the BaseOptionsList here.

There's a couple things that could make it true. I'm going to compare dev to prod.

On dev I see that the focusedIndex is 0, making this focused. On prod the focusedIndex is 1, so it's not focused.

[neil-marcellini]

I'm pretty sure focusedIndex is the root cause, but now the question is why? Hopefully we can find where that was changed recently.

[paultsimura]

@neil-marcellini I think this might be coming from #31606 – there were a ton of changes to the BaseOptionsSelector, and you're right – the selectedIndex is messed up on dev compared to prod.

[neil-marcellini]

Ah yes good call, that PR likely caused the problem

[neil-marcellini]

We have two options, we can either revert that PR, or put up a fix.

The root cause of the problem is that we have an effect on the sections, and when this line runs we set the focusedIndex to 0 which is the newFocusedIndex.

App/src/components/OptionsSelector/BaseOptionsSelector.js

Line 434 in d561cf7

setFocusedIndex(prevFocusedOptionIndex || (_.isNumber(props.focusedIndex) ? props.focusedIndex : newFocusedIndex));

Here's a log line from debugging

Set focusedIndex, section effect, prevFocusedOptionIndex = undefined, focusedIndex = undefined, newFocusedIndex = 0

On prod we currently have this in a componentDidUpdate block and so it does not run on the initial render, in contrast to the effect.

[neil-marcellini]

I'll ask in Slack about reverting. It's a big PR and would suck to have to revert, but already seems to be causing lots of problems. The problematic code here doesn't make much sense to me anyways, so I think we really need to refactor/re-evaluate the whole component.

[dragnoir]

@neil-marcellini I understand that you was comparing with the function before the new recent merge and you want the code inside the useEffect to run only after sessions update because this was in a componentDidUpdate block and so it does not run on the initial render.

But it was broken, pls check the video here to see how it was broken and it's fixed now.

I will explain below in more details:
Please check this image:

on the Choose a workspace screen, the props.selectedOptions is the list of the options selected, in this case it's just one option.
We need to run inside the useEffect and calculate the new focusedIndex with newFocusedIndex = props.selectedOptions.length to point the selected item into the latest item not selected (available for selection)

In a case with many items (UI can show those on the top), going with the choosen proposal will broke the code (focusedIndex wrong) and the selected item will be always from the selected items. We need to go throw the props.selectedOptions and check:

• If we have selectedOptions, then the focused index go to the latest item wish is the 1st one available for selection
• if props.selectedOptions is empty, so nothing selected, focusedIndex point to the 1st item on the list.

[paultsimura]

But it was broken, pls check the video here to see how it was broken and it's fixed now.

To me, this looks like you just decided that the way it's "working" now is correct. I don't recall any issue that would request such a logic to be implemented. It doesn't even make sense to highlight the option next to the selected one by default, because it will lead to this confusing UX:

1. user opens a list with already selected option
2. doesn't change anything, but presses enter
3. the next option gets selected because it was automatically highlighted

Why?

[dragnoir]

@paultsimura pls check the video I posted comparing between prod and stag, the 1st one, you can't use keyboard to navigate on prod. It's broken on prob and fixed on stag.

[paultsimura]

It's broken on prob and fixed on stag.

It's not fixed on staging - it's just replaced with another bug. On staging, you still cannot move with the keyboard up to the "Expensify" option. We may want to fix this, but that would be a separate issue that will address this bug correctly.

Moreover @dragnoir, you saw the issue #37239 which explicitly states that the behavior you identify as correct (the second half of your recording) is a bug. Yet you're trying to convince the opposite in both this and that issue tagging Engineers and C+. I'll let @neil-marcellini handle the further discussion here.

[neil-marcellini]

Yes @dragnoir I think it's all getting a bit murky, and I agree with @paultsimura's reasoning. I think we have a pretty clear problem and solution for this specific issue. There are other issues I'm sure, but let's solve those elsewhere.

[neil-marcellini]

The fix was merged and I requested it to be cherry picked to staging.

[melvin-bot]

⚠️ 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.

[Piotrfj]

Update: re-refactoring is ready to be tested, it fixes all the regression issues I was aware of. Please test it (especially the tags, because I could not yet replicate the scenario on my site).
The fix is based on the @roryabraham solution, so special thanks for him!
Link to PR: #37494

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

• fix: Recalculate the sections only on actual change #37341

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

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

• @akinwale requires payment automatic offer (Reviewer)
• @paultsimura requires payment automatic offer (Contributor)

[melvin-bot]

Issue is ready for payment but no BZ is assigned. @sonialiap you are the lucky winner! Please verify the payment summary looks correct and complete the checklist. Thanks!

[akinwale]

@sonialiap Bump for payment. Thanks.

[sonialiap]

@neil-marcellini I see there was a deploy blocker, did this issue cause regressions?

[akinwale]

@sonialiap This issue was a deploy blocker itself with the PR cherry-picked.

The issue was linked in the other deploy blocker since it was related. Some context.

[neil-marcellini]

Yes @akinwale is correct, this issue fixed regressions, it has not created any.

[sonialiap]

Thanks for the confirmation! Both payments made

@akinwale Reviewer $500 ✔️
@paultsimura Contributor $500 ✔️