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

• [x] Android: Native
• [x] Android: mWeb Chrome
• [x] iOS: Native
• [x] iOS: mWeb Safari
• [x] MacOS: Chrome / Safari
• [x] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/93399543/956c7e3d-63ac-4c87-adf0-492f0a7c0c92

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[bot]]

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

[melvin-bot[bot]]

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

[github-actions[bot]]

:wave: 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[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 https://github.com/Expensify/App/pull/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 https://github.com/Expensify/App/pull/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.

https://github.com/Expensify/App/blob/d561cf750d41cdec8f7180890cbdb5b8c666e2f7/src/components/OptionsSelector/BaseOptionsSelector.js#L434

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.

[neil-marcellini]

We decided in Slack that the best thing is to find a solution to this issue vs reverting that large PR. @paultsimura would you like to come up with a solution for the root cause?

[BhuvaneshPatil]

Proposal

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

IOU - "To" destination is highlighted in confirmation page

What is the root cause of that problem?

The root cause is refactor of base option selector PR. https://github.com/Expensify/App/pull/31606 If we want to dive deep in, the issues is caused here - https://github.com/Expensify/App/blob/d2a9c6faa9f450c20037a968974844f4f4325198/src/components/OptionsSelector/BaseOptionsSelector.js#L428-L434

As selectedOptions is empty array, focussed index is 0.

https://github.com/Expensify/App/blob/d2a9c6faa9f450c20037a968974844f4f4325198/src/components/MoneyTemporaryForRefactorRequestConfirmationList.js#L461-L466

In case of money request it's an empty array.

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

We can pass - focusedIndex={-1} for OptionsSelector

 <OptionsSelector
....
focusedIndex={-1}
...

in MoneyTemporaryForRefactorRequestConfirmationList.

In case of Split Bill the item isn't focused, so we can safely pass focusedIndex={-1}. But if we want to make sure we only touch money request, we can do -

focusedIndex={!hasMultipleParticipants ?-1 : undefined}

What alternative solutions did you explore? (Optional)

[BhuvaneshPatil]

@neil-marcellini I can make the PR right away if you think the solution looks good to you.

[paultsimura]

Proposal

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

The item on 0-position gets focused on the first render of the BaseOptionsSelector

What is the root cause of that problem?

This hook runs on every change of the sections and on the first render, while it's supposed to run only when the sections indeed change:

https://github.com/Expensify/App/blob/fd0302e8db1eb9ab317242463beee973414a1f84/src/components/OptionsSelector/BaseOptionsSelector.js#L417-L437

As a result, we set focusedIndex to newFocusedIndex = 0 here:

https://github.com/Expensify/App/blob/fd0302e8db1eb9ab317242463beee973414a1f84/src/components/OptionsSelector/BaseOptionsSelector.js#L434

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

We want this hook to run only when the sections change, so we should make use of the usePrevious hook (or useRef as we use for other prev* values in this component):

+    const previousSections = usePrevious(props.sections);
...
    useEffect(() => {
+        if (lodashIsEqual(props.sections, previousSections)) {
+            return;
+        }
        const newSections = sliceSections();
        const newOptions = flattenSections();

        if (prevLocale.current !== props.preferredLocale) {
            prevLocale.current = props.preferredLocale;
            setAllOptions(newOptions);
            setSections(newSections);
            return;
        }

        const newFocusedIndex = props.selectedOptions.length;
        const prevFocusedOption = _.find(newOptions, (option) => focusedOption && option.keyForList === focusedOption.keyForList);
        const prevFocusedOptionIndex = prevFocusedOption ? _.findIndex(newOptions, (option) => focusedOption && option.keyForList === focusedOption.keyForList) : undefined;

        setSections(newSections);
        setAllOptions(newOptions);
        setFocusedIndex(prevFocusedOptionIndex || (_.isNumber(props.focusedIndex) ? props.focusedIndex : newFocusedIndex));
        // we want to run this effect only when the sections change
        // eslint-disable-next-line react-hooks/exhaustive-deps
+    }, [props.sections, previousSections]);
-    }, [props.sections]);

What alternative solutions did you explore? (Optional)

[dragnoir]

Proposal

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

1st item should not be highlighted.

What is the root cause of that problem?

The first item is highlighted because we are passing an empty value to selectedOptions https://github.com/Expensify/App/blob/cfa0ae37156208ebecdc6b2126464ec462271b76/src/components/MoneyTemporaryForRefactorRequestConfirmationList.js#L854

Due to this empty, then inside BaseOptionsSelector there's a check

const  newFocusedIndex  =  props.selectedOptions.length;

because selectedOptions is empty, the value of focusedIndex is now from 1 to 0.

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

We need to pass the list of the options already selected, something like:

selectedOptions={selectedParticipants}

POC video:

https://github.com/Expensify/App/assets/12425932/b7ff8135-b4fd-4549-b1d8-e827e088ee5e

[ntdiary]

Just adding a note, perhaps we need also consider an edge case: fields in the sections may be updated by the backend push, resulting in the focusedIndex state being updated. 😂

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

[dragnoir]

@ntdiary @neil-marcellini I would like to note that there's no regression from https://github.com/Expensify/App/pull/31606 The new update is good and the component behave as it it. First item from the list to be highlighted is the default and the correct behavior.

Please check on the video:

• on the right: the new.expensify, where the old BaseOptionsSelector, I am not able to use keyboard to navigate throw the list
• on the left, I can naviagte throw the list and the navigation start from the 1st item I can select (enter from keyboard works good)

Video:

https://github.com/Expensify/App/assets/12425932/e0109eee-d49c-4b80-9993-19bf9d7a19ce

As for this issue, pls check the console log displaying the selectedOptions for 2 cases: 1- this issue, within MoneyTemporaryForRefactorRequestConfirmationList 2- when we select a list for split bill

https://github.com/Expensify/App/assets/12425932/7dfaad5d-78b0-4e0f-b089-29bd5066672a

[neil-marcellini]

I like @paultsimura's proposal best since it best matches the behavior on prod and properly addresses the root cause of the issue. It seems like selectedOptions should be empty, in contrast to suggestions in other proposals, because the user never selected any options.

[melvin-bot[bot]]

📣 @akinwale 🎉 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[bot]]

📣 @paultsimura 🎉 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 📖

[dragnoir]

@neil-marcellini I tested the accepted proposal and it doesn't works!! Same issue and same result on the app.

The useEffect hoof will runs after the initial render and after re-renders with changed dependencies.

The value of the focusedIndex will be edited on the initial render even if we campare previous and new sections.

[paultsimura]

@dragnoir could you please attach a recording here? Because in the PR, you can already find an attached recording that proves the fix works.

[dragnoir]

@paultsimura I checked your PR, not passing throw the function break the Choose a workspace page. Try to use keyboard arrows, you will not be able to navigate. Also the selected item will be a wrong one or none.

[paultsimura]

Thanks, let me double-check this 🤔

[paultsimura]

Thanks @dragnoir, that was another regression from https://github.com/Expensify/App/pull/31606 – added the fix into the PR.

[ikevin127]

...from https://github.com/Expensify/App/issues/36577:

@ ntdiary: https://github.com/Expensify/App/issues/36577#issuecomment-1959744733:

Actually, focusedIndex doesn't seem to be used on this confirm page, so some possible directions are: how to keep it unchanged when re-rendering, or whether it can be disabled, etc.

... https://github.com/Expensify/App/issues/36577#issuecomment-1966508350:

so far, @ikevin127's https://github.com/Expensify/App/issues/36577#issuecomment-1946332354 is a straightforward solution, but I'm still willing to wait a bit longer, There might be potentially better ideas. BTW, this issue is somewhat similar to issue https://github.com/Expensify/App/issues/37238, so I will also discuss it there. :)

@ ikevin127 https://github.com/Expensify/App/issues/36577#issuecomment-1967517205:

This issue's root cause is related to OpenPublicProfilePage being called which updates focusedIndex because props.sections[0].data[0].participantsList[0].isLoading changes its boolean value, while the other issue's accepted RCA was pretty vague, mentioning that the sections prop changes and suggested to compare previous / current sections as a fix.

Turns out the fix PR from https://github.com/Expensify/App/issues/37238 selected proposal didn't work as expected and a regression was reported.

I think regardless of the root cause of the other issue, my latest proposed solution would fix both issues since as you mentioned above:

Actually, focusedIndex doesn't seem to be used on this confirm page

To me, this looks like a straight-forward slam dunk 🏀 I'm confused as to what's the hold up w/ this one really 🤔

[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 https://github.com/Expensify/App/issues/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[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: https://github.com/Expensify/App/pull/37494

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

• https://github.com/Expensify/App/pull/37341

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

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[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 ✔️