[HOLD component refactoring] [$500] [LOW] Green circular loading does not appear when searching for a contact in request money flow

[m-natarajan]

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.43-14
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: @brunovjk
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1708081508978019

Action Performed:

1. Click + and select Request money
2. Enter amount and click next
3. Search for any contact in the field

Expected Result:

Green circular loading ring is seen like when we search for a contact to chat

Actual Result:

Not appearing when searching in Request money flow

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

Recording.2773.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01788bcdaa345e5022
• Upwork Job ID: 1761057209378701312
• Last Price Increase: 2024-02-23

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01788bcdaa345e5022

[melvin-bot]

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

[melvin-bot]

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

[FitseTLT]

Proposal

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

Green circular loading does not appear when searching for a contact in request money flow

What is the root cause of that problem?

We are not passing isLoadingNewOptions prop to the selection list for participants here

App/src/pages/iou/request/MoneyTemporaryForRefactorRequestParticipantsSelector.js

Lines 338 to 339 in 29ad878

	<SelectionList
	onConfirm={handleConfirmSelection}

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

We should be passing isLoadingNewOptions prop to the selection list as for search page here

App/src/pages/SearchPage/index.js

Lines 169 to 170 in 29ad878

	isLoadingNewOptions={isSearchingForReports}
	/>

after subscribing to it as in here

App/src/pages/SearchPage/index.js

Lines 189 to 193 in 29ad878

	isSearchingForReports: {
	key: ONYXKEYS.IS_SEARCHING_FOR_REPORTS,
	initWithStoredValues: false,
	},
	})(SearchPage);

What alternative solutions did you explore? (Optional)

[brunovjk]

Proposal

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

When making a Request money, the circular loading does not appear when we search for contact

What is the root cause of that problem?

We are not setting isLoadingNewOptions correctly in MoneyTemporaryForRefactorRequestParticipantsSelector like we do in SearchPage.

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

We need to get isSearchingForReports from onyx and use it in MoneyTemporaryForRefactorRequestParticipantsSelector SelectionList

What alternative solutions did you explore? (Optional)

[dukenv0307]

Proposal

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

1. Green circular loading not appearing when searching in Request money flow
2. Skeleton loading appears when searching in the Send money flow but doesn't appear in the Request money flow

What is the root cause of that problem?

1. We don't have isLoadingNewOptions={isSearchingForReports} in here like the NewChatPage, so the loading indicator does not show.

2. In the Send money flow here, we have the Skeleton loading whenever the user types something to search in the search input, this is inconsistent with the Request money flow where the Skeleton doesn't appear in this case (it only appears initially when first loading the list). This is because in Request money we have a different logic to show the skeleton placeholder based on the didScreenTransitionEnd and isOptionsDataReady here

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

1. We need to connect to Onyx ONYXKEYS.IS_SEARCHING_FOR_REPORTS there and adds isLoadingNewOptions={isSearchingForReports} here

We need to do the same thing in the MoneyRequestParticipantsSelector too (For the Send money flow)

2. If we want the behavior in Request money page (not showing skeleton placeholder when searching), in Send money page we should update this to follow the same logic as in Request money page here. If instead we want to show skeleton placeholder when searching like Send money page, update request money page here to use isSearchingForReports similar to Send money page.

What alternative solutions did you explore? (Optional)

NA

[mollfpr]

Just a head-ups! I'll be reviewing the proposal from @brunovjk first since the posted the proposal along side with this bug report. Context: https://expensify.slack.com/archives/C01GTK53T8Q/p1708081508978019

[jliexpensify]

Posted to the Wave6 channel - https://expensify.slack.com/archives/C02MW39LT9N/p1708908540463509

[mollfpr]

Thank you guys for your patience!

The proposal from @brunovjk looks good to me! I'm also inclined to update MoneyRequestParticipantsSelector to have isLoadingNewOptions, so we can get the same behavior for the send money flow.

🎀 👀 🎀 C+ reviewed!

[melvin-bot]

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

[dukenv0307]

@mollfpr There's also a similar issue with the loading skeleton in both pages (number 2 in my proposal). The selected proposal won't fix that.

@mollfpr Can you provide some feedback on my proposal, which fixes both issues.

Thank you!

[mollfpr]

My consideration is the main issue here is the MoneyTemporaryForRefactorRequestParticipantsSelector missing isLoadingNewOptions. So basically the proposal from @brunovjk has solved the issue, and the other proposal seems doing the same.

Yes, your proposal has an addition to fix the send money flow, but because it's not the main issue, the fix is pretty similar. So I don't think it's fair for another contributor that focus only on the main problem here.

I let @lakchote decide who's gonna work on this issue.

[dukenv0307]

Yes, your proposal has an addition to fix the send money flow, but because it's not the main issue, the fix is pretty similar. So I don't think it's fair for another contributor that focus only on the main problem here.

@mollfpr I think proposals should be as thorough as possible and not solely limited to only the issue's OP, we don't want to leave bugs behind in the flows we're fixing. The fix is completely different IMO since it's related to the Skeleton, not the Green loading indicator, and it doesn't use isSearchingForReports. It also is not something that could be discovered and fixed during PR review so could be considered substantially different from the selected proposal.

• If we decide to fix only the main issue of Green circular loading here, I agree it'd be fair to go with the selected proposal.
• If we decide to fix both issues regarding loading behavior in the pages as stated in my proposal. I think my proposal would be a better and more fair choice (without it the 2nd issue will never be discovered and fixed).
  (A similar case was here where we want to leaves the app/code in a better place than it was before)

Let's wait for @lakchote 's take on the scope of this issue and who to assign.

[brunovjk]

@mollfpr Thanks for the review.

In the Send money flow here, we have the Skeleton loading whenever the user types something to search

I didn't mention this because I thought it was out of scope of this issue. I dont think we are going to use MoneyRequestParticipantsSelector. If I'm not mistaken, we are still refactoring SendMoney flow and we can deal with that later :D
I have context on this transition and refactoring: #32665, That's why I reported this bug in the first place. I can "update MoneyRequestParticipantsSelector to have isLoadingNewOptions, so we can get the same behavior for the send money flow." if deemed necessary.

[dukenv0307]

If I'm not mistaken, we are still refactoring Selection List SendMoney flow and we can deal with that later :D

We won't remember/keep track of this to do it. Better to have correct behavior before the refactor, and keep the behavior correct after the refactor.

Let's minimize discussions and wait for @lakchote's final decision on issue scope and assignment

[melvin-bot]

Current assignee @jliexpensify is eligible for the NewFeature assigner, not assigning anyone new.

[lakchote]

I do agree about splitting the issue to be specific on the problem we're solving.

Let's solve them both.

I've put this on hold then, and once the new component will be in place we'll resume work for it.

[brunovjk]

Pages that use SelectionList to handle a options list	Skeleton	Green Circular
- Search page	✅ Search.page.-.skeleton.mp4	✅ Search.page.-.circular.mp4
- New chat	❌ should be fixed here New.chat.-.skeleton.mp4	✅ New.chat.-.circular.mp4
New room chat - RoomInvitePage - Press "Start chat" - Select "Room" - Fill the name and create a room - Open room details - Press "Members" - Press "Invite"	❌ New.room.chat.-.skeleton.mp4	❌ New.room.chat.-.circular.mp4
- Request money	❌ Request.money.-.skeleton.mp4	❌ Request.money.-.circular.mp4
- Send money	❌ Send.money.-.skeleton.mp4	❌ Send.money.-.circular.mp4
- Assign task	❌ Assign.task.-.skeleton.mp4	❌ Assign.task.-.circular.mp4
Assign task - share somewhere - Press "Assign task" - Fill the title and press "Next" - Press "Share somewhere"	❌ Assign.task.-.share.somewhere.-.skeleton.mp4	✅ Assign.task.-.share.somewhere.-.circular.mp4
Invite to workspace - Open "Choose a workspace" - Press "+" button - Go to "Members" - Press "Invite member"	❌ Invite.to.workspace.-.skeleton.mp4	❌ Invite.to.workspace.-.circular.mp4
Share logs - Go to Settings -> About -> Troubleshoot - Enable "Client Side logging" - Press "Debug console" - Press "Share log"	❌ Share.logs.-.skeleton.mp4	❌ Share.logs.-.circular.mp4

1. I think the Skeleton issue still requires hold. Can we fix them all together in a separate issue/PR later? Since it has different root causes (didScreenTransitionEnd + "loading searchable lists by caching the options") from this issue.

2. For the Green Circular loading (this issue), I can updat my PR to include all cases. NewChatPage still uses OptionsSelector we could wait Replace OptionsSelector with SelectionList to move on, but I think we are good here, since the green circular on the NewChatPage is working now and should keep working after refactor.

@mollfpr @lakchote @mountiny @dukenv0307 Thoughts?

[mountiny]

I am up for unifying this

[lakchote]

I am up for unifying this

I agree with @mountiny 👍

[brunovjk]

Sorry but what do you mean by 'unifying this'? Are we focusing on all instances of Green Circular or merging Skeleton and Green Circular issues?

[dukenv0307]

I think:
@brunovjk can raise the PR to fix all Green Circular cases now.

And I can raise a subsequent PR to fix all the Skeleton cases later when it's off hold.

[jliexpensify]

@mountiny and @lakchote if this is fine, I assume we'll have 2x $500 payments for @brunovjk and @dukenv0307?

[mountiny]

Unifying means as you wrote:

For the Green Circular loading (this issue), I can update my #37842 to include all cases.

Just making sure the green loading works same everywhere we would expect this

[brunovjk]

Im on it

[brunovjk]

PR ready for review

[melvin-bot]

This issue has not been updated in over 15 days. @lakchote, @mollfpr, @jliexpensify, @brunovjk eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[brunovjk]

PR was merged, but not to staging yet

Edit: PR on staging PR on production

[jliexpensify]

Payment Summary

• C: @brunovjk $500
• C: @dukenv0307 $500 (NO PAYMENT DUE YET)
• C+: @mollfpr $500 (NEW.DOT PAYMENT)

NEW UPWORKS JOB

[lakchote]

Payment Summary

• C: @brunovjk $500
• C: @dukenv0307 $500
• C+: @mollfpr $500

Is this correct? I'll need to create a new Upworks job.

This is correct for @mollfpr and @brunovjk! However not for @dukenv0307 as his PR for handling skeleton cases isn't ready, so no payment is due yet.

[jliexpensify]

@lakchote, thanks for checking! I assume I will be paying @dukenv0307 via this job, so we'll need to keep this open?

[jliexpensify]

NEW UPWORKS JOB

[jliexpensify]

Paid @brunovjk

[JmillsExpensify]

$500 approved for @mollfpr

[jliexpensify]

Note for myself: waiting to pay @dukenv0307

[jliexpensify]

@lakchote feel free to bump me when @dukenv0307 needs payment.

[brunovjk]

@lakchote What do you think about close this issue and open another suitable one to deal with skeleton loading in SelectionList?
Keep things organized. Since they have different causes, solutions, assignee and payments. I can help with the issue report if needed.

[melvin-bot]

@lakchote, @mollfpr, @jliexpensify, @brunovjk, this Monthly task hasn't been acted upon in 6 weeks; closing.

If you disagree, feel encouraged to reopen it -- but pick your least important issue to close instead.