[$250] Expense - On selecting a category/ tags by multiple tapping, user directed to conversation page

[IuliiaHerets]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: V9. 0.69-0 Reproducible in staging?: Y Reproducible in production?: Y Issue reported by: Applause Internal Team

Action Performed:

1. Go to https://staging.new.expensify.com/home
2. Go to workspace settings
3. Enable tags
4. Create few tags
5. Tap workspace chat
6. Create an expense with category and tag
7. Open the expense
8. Open category and select a category by multiple tapping
9. Open tag and select a tag by multiple tapping

Expected Result:

On selecting a category/ tags by multiple tapping, user must not be directed to conversation/merchant page instead of expense details page.

Actual Result:

On selecting a category/ tags by multiple tapping, user directed to conversation/merchant page instead of expense details page.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

https://github.com/user-attachments/assets/fd4c73c4-0736-4313-a477-b15f23c9660c

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021864001016141250370
• Upwork Job ID: 1864001016141250370
• Last Price Increase: 2024-12-10

Issue Owner

Current Issue Owner: @alitoshmatov

[melvin-bot[bot]]

Triggered auto assignment to @puneetlath (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[Krishna2323]

Edited by proposal-police: This proposal was edited at 2024-11-30 21:59:49 UTC.

Proposal


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

Expense - On selecting a category/ tags by multiple tapping, user directed to conversation page

What is the root cause of that problem?

• The tag/category option can be clicked/pressed twice until the page is completely removed and when we press twice, the updateTag function is called twice and navigates back twice. https://github.com/Expensify/App/blob/3cc88f5a83a979c5e588e01065083a3d714e69da/src/pages/iou/request/step/IOURequestStepTag.tsx#L74-L94

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


• We should return early from updateTag or similar functions on other selection list page if the isFocused state from @react-navigation/native is false.
• If we want we can apply this logic directly in SelectionList & SelectionListWithModal

What alternative solutions did you explore? (Optional)

• We can apply this logic in ScreenWrapper and few other selector modals by using pointerEvents prop. This way, press events won't be triggered when screen is not focused.
• We can also add a prop in ScreenWrapper to apply this logic conditionally.

            <View
                ref={ref}
                style={[styles.flex1, {minHeight}]}
                // eslint-disable-next-line react/jsx-props-no-spreading, react-compiler/react-compiler
                {...panResponder.panHandlers}
                testID={testID}
                pointerEvents={isFocused ? 'auto' : 'none'}
            >

https://github.com/Expensify/App/blob/3cc88f5a83a979c5e588e01065083a3d714e69da/src/components/ScreenWrapper.tsx#L262-L268

Result

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[dominictb]

Edited by proposal-police: This proposal was edited at 2024-12-04 10:01:24 UTC.

Proposal

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

Expense - On selecting a category/ tags by multiple tapping, user directed to conversation page

What is the root cause of that problem?

When updating tag, we'll redirect to previous page

https://github.com/Expensify/App/blob/ce282d38777e9e0a267e1b6f44bff9c4aace98ee/src/pages/iou/request/step/IOURequestStepTag.tsx#L78

If users press twice quickly, this function will be triggered 2 times so we call goBack 2 times

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

We shouldn't lean on isFocused since we still can call onSelectRow multiple times when the page is focused.

We should fix this issue on BaseSelectionList to prevent the bug like this.

1. Define the new prop called shouldCloseListAfterSelectingRow, false by default. Then enable it in case we want to close list (by navigation,...).
2. Creating isExecuting ref, then update it in selectRow function

    const isExecuting = useRef(false);
 const selectRow = useCallback(

        (item: TItem, indexToFocus?: number) => {
            if(shouldCloseListAfterSelectingRow){
                if(isExecuting.current){
                    return;
                }
                isExecuting.current = true
        }
...

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

What alternative solutions did you explore? (Optional)

N/A

[dominictb]

@alitoshmatov Can you take a look at my proposal? Thanks

[melvin-bot[bot]]

@puneetlath, @alitoshmatov Eep! 4 days overdue now. Issues have feelings too...

[melvin-bot[bot]]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[puneetlath]

Bumped @alitoshmatov in Slack.

[melvin-bot[bot]]

@puneetlath, @alitoshmatov Still overdue 6 days?! Let's take care of this!

[alitoshmatov]

@Krishna2323 Can you recheck your solution, I am still able to reproduce the issue

[Krishna2323]

@alitoshmatov, the solutions works well. Can you please tell me the platform you are testing on? FYI, the isFocused solution is already applied in SelectionListWithModal. https://github.com/Expensify/App/blob/bcb864181fb1a9825ce32b3cb16ec6c0a6d00c31/src/components/SelectionListWithModal/index.tsx#L81-L86

https://github.com/user-attachments/assets/22f7b51b-5db8-4b62-9eaa-f09e0b58099d

[alitoshmatov]

Oh, my bad, I was changing tag component and testing category. Now it is working.

[alitoshmatov]

Thank you @dominictb for proposal

We shouldn't lean on isFocused since we still can call onSelectRow multiple times when the page is focused.

Can you elaborate, are you suggesting that we can still reproduce the issue with @Krishna2323 proposal.

In your solution it seems that after first selection you just disable selecting row, can it cause some issue where we might allow another selection after first one?

[melvin-bot[bot]]

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

[dominictb]

@alitoshmatov

Can you elaborate, are you suggesting that we can still reproduce the issue with @Krishna2323 proposal.

Yes, we can still can reproduce, but it's quite hard. Navigating is the async action, so we can perform multiple actions while navigating (We can't guarantee isFocus is false after the first click).

In your solution it seems that after first selection you just disable selecting row, can it cause some issue where we might allow another selection after first one?

This way won't cause the regression since shouldCloseListAfterSelectingRow is false by default, we just enable it in case we want close the list after selecting row.

[Krishna2323]

Yes, we can still can reproduce, but it's quite hard. Navigating is the async action, so we can perform multiple actions while navigating (We can't guarantee isFocus is false after the first click).

I don't think that's correct because the same solution has been applied in multiple places, and we have never faced the issue again. I don't think this issue can even be reproduced after applying the solution.

[melvin-bot[bot]]

@puneetlath, @alitoshmatov Huh... This is 4 days overdue. Who can take care of this?

[melvin-bot[bot]]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[melvin-bot[bot]]

@puneetlath, @alitoshmatov 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[puneetlath]

@alitoshmatov can you summarize where we're at with this issue?

[alitoshmatov]

I think we can go with @Krishna2323 's proposal. I think isFocused is a reliable state here.

C+ reviewed 🎀 👀 🎀

[melvin-bot[bot]]

Current assignee @puneetlath is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[dominictb]

@alitoshmatov @Krishna2323 In case we want to prevent multiple tapping, we had the same logic to do that in useSingleExecution and RightModalNavigator. We used ref to make sure this action is only executed 1 time. I believe we should use the same technique instead of leaning on isFocus.

As I said above, navigation is the async action so we can't make sure after the first call it changes to false immediately.

If we can point out the reason to use isFocus instead of ref, that would be clear to me. Thanks cc @puneetlath

[melvin-bot[bot]]

@puneetlath, @alitoshmatov Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[Krishna2323]

We use isFocused state to handle similar behaviour in many places and I don't see any issues with that. If you can reproduce the issue with the isFocused state solution then it will be easy for us to understand you concern.

Here are few components which uses isFocused state to disable press on elements: https://github.com/Expensify/App/blob/5f65aecfccc04483bb4bf07852ecf07ee05e6081/src/components/Button/index.tsx#L180 https://github.com/Expensify/App/blob/5f65aecfccc04483bb4bf07852ecf07ee05e6081/src/components/SelectionList/BaseSelectionList.tsx#L318-L323 https://github.com/Expensify/App/blob/5f65aecfccc04483bb4bf07852ecf07ee05e6081/src/pages/WorkspaceSwitcherPage/index.tsx#L81-L86 https://github.com/Expensify/App/blob/bcb864181fb1a9825ce32b3cb16ec6c0a6d00c31/src/components/SelectionListWithModal/index.tsx#L81-L86

[dominictb]

In case we want to prevent multiple tapping, we had the same logic to do that in useSingleExecution and RightModalNavigator. We used ref to make sure this action is only executed 1 time. I believe we should use the same technique instead of leaning on isFocus. As I said above, navigation is the async action so we can't make sure after the first call it changes to false immediately.

Here's what I think we should use ref instead of isFocus. Let's see @puneetlath's though

[Krishna2323]

FYI, in this PR, we resolved a similar bug in another component using the isFocused state. We never encountered any issues during the testing, and we also wrote tests for it, which passed successfully.

[melvin-bot[bot]]

@puneetlath, @alitoshmatov Eep! 4 days overdue now. Issues have feelings too...

[melvin-bot[bot]]

@puneetlath, @alitoshmatov Still overdue 6 days?! Let's take care of this!