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

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 could you manually make the offers for @mollfpr and @brunovjk please.

Unifying means as you wrote:

For the Green Circular loading (this issue), I can update my https://github.com/Expensify/App/pull/37842 to include all cases.

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

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.

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?

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.

Hey @lakchote, @mountiny and @dukenv0307,

Appreciate you delving into this with me. 🚀 Still getting the ropes here, and your insights are a huge help.

How will it solve it? I do not agree. In the new component, there’s the old logic in place, see here.

The line you referenced is from our PR. Also, I didn’t move the new change to the old component based on @ntdyary’s suggestion link to the comment.

Please, correct me if Im wrong. In my view, we’ve got two distinct issues: green circular loading (our current discussion) and skeleton loading control (highlighted by @dukenv0307). Our issue involves the variable isSearchingForReports controlling isLoadingNewOptions for green circular loading. @dukenv0307issue deals with didScreenTransitionEnd and isOptionsDataReady controlling showLoadingPlaceholder for skeleton loading. The former triggers with every new contact search, while the latter activates only once during screen transition/list loading.

Currently, we have several flows loading contact lists using IOURequestStepParticipants (new component) and MoneyRequestParticipantsPage (old component), Search, Start chat, Request money manual, Send money, Assign task ,… The transition/skeleton error was reported here, and during the PR, it surfaced in both components, as indicated by @ntdyary (link to the comment). However, he suggested not altering the old component as it’s slated for removal.

In summary:

1. The skeleton loading issue is addressed in my latest PR, focusing on MoneyTemporaryForRefactorRequestParticipantsSelector. I see now that we should have dealt with this at once. Ensuring consistency between MoneyRequestParticipantsSelector and MoneyTemporaryForRefactorRequestParticipantsSelector might come with component refactoring, but your guidance suggest otherwise.
2. The green circular loading issue could be a separate task (I can update my PR to modify MoneyRequestParticipantsSelector as well).
3. Ensure consistency in the use of (IOURequestStepParticipants and MoneyRequestParticipantsPage).

I dont mind about splitting compensation. I just believe it’d be more effective to compartmentalize these into distinct purposes/issues.

@lakchote @brunovjk Given that this issue would prevail in the new component, I think we should put this issue on hold for the other one and then we can resume work once new component will be in place

@dukenv0307 The component you are trying to change will be removed here, and after “Replace all uses of the Old Component with the New Component” the issue you are reporting will not exist.

How will it solve it? I do not agree. In the new component, there’s the old logic in place, see here.

Since no changes have been made to the logic in the old component, it won’t be ported to the new component. So the issue will remain.

If we just solve the current issue and not the related one for the skeleton, we risk letting a known problem be unfixed whereas we could act on it. That’s not ideal actually. We want to have the same behavior across components. Especially related ones (Request/Send money components). The “other” issue reported by @dukenv0307 was found while he was investigating the bug. It’s not like the two issues are entirely unrelated.

I’m going to seek for an external opinion on this. Thanks @dukenv0307 for pinging me again on this and raise my awareness of the problem.

cc @mountiny

@brunovjk’s proposal LGTM.

Thanks @mollfpr for the C+ review.

DM’ed @lakchote for the review!

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.

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.

@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!

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!