[$250] mWeb - Selector - User lands on the same WS when double tapping on different one on selector

[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: 9.0.53-0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/5122323&group_by=cases:section_id&group_order=asc&group_id=296775 Issue reported by: Applause Internal Team

Action Performed:

1. Open the staging.new.expensify.com website.
2. Tap on Workspace selector on the top left corner.
3. Change to any workspace on the list.
4. Double tap on Expensify option.
5. Verify that all the chats are displayed in LHN.
6. Open Workspace selector again.
7. Double tap on the next workspace to the one selected.
8. Verify that the chats of the selected workspace are displayed.

Expected Result:

LHN should display the selected workspace chats when the user double taps over it in selector.

Actual Result:

User remains on the same workspace when trying to select "Expensify" option or next workspace on the selector list with double tapping.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

https://github.com/user-attachments/assets/a999e26e-995f-455b-bbcb-868eb60a0e02

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021849976484014852041
• Upwork Job ID: 1849976484014852041
• Last Price Increase: 2024-11-09
• Automatic offers:
• Krishna2323 | Contributor | 104866093

Issue Owner

Current Issue Owner: @s77rt

[melvin-bot[bot]]

Triggered auto assignment to @stephanieelliott (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.

[IuliiaHerets]

We think that this bug might be related to #wave-collect - Release 1

[IuliiaHerets]

@stephanieelliott FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[nyomanjyotisa]

Proposal

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

Selector - User lands on the same WS when double tapping on different one on selector

What is the root cause of that problem?

On double click, only Navigation.goBack() executed on the second click, and the Navigation.navigateWithSwitchPolicyID({policyID}) not executed since policyID !== activeWorkspaceID is true on second click

https://github.com/Expensify/App/blob/09f19c3ad08b269db9821bb9c4eb45152bd9b7cb/src/pages/WorkspaceSwitcherPage/index.tsx#L85-L100

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

Implement debouncing to prevent double click

    const selectPolicy = useCallback(
        debounce((option?: WorkspaceListItem) => {
            console.log("set policy")

            if (!option) {
                return;
            }

            const {policyID} = option;

            setActiveWorkspaceID(policyID);
            Navigation.goBack();
            if (policyID !== activeWorkspaceID) {
                Navigation.navigateWithSwitchPolicyID({policyID});
            }
        }, 300),
        [activeWorkspaceID, setActiveWorkspaceID],
    );

What alternative solutions did you explore? (Optional)

[Krishna2323]

Edited by proposal-police: This proposal was edited at 2024-10-27 22:00:21 UTC.

Proposal


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

mWeb - Selector - User lands on the same WS when double tapping on different one on selector

What is the root cause of that problem?

• Double navigation is happening because the Navigation.goBack(); & Navigation.navigateWithSwitchPolicyID({policyID}); is called twice when double clicking. https://github.com/Expensify/App/blob/81e877a3e75e51fc19a14e43e4632d3889949584/src/pages/WorkspaceSwitcherPage/index.tsx#L85-L100

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


• We can use useIsFocused hook to check if the screen is focused or not and if the screen is not focused we will return early from selectPolicy .

    const isFocused = useIsFocused();

    const selectPolicy = useCallback(
        (option?: WorkspaceListItem) => {
            if (!option || !isFocused) {
                return;
            }

            const {policyID} = option;

            setActiveWorkspaceID(policyID);
            Navigation.goBack();
            if (policyID !== activeWorkspaceID) {
                Navigation.navigateWithSwitchPolicyID({policyID});
            }
        },
        [activeWorkspaceID, setActiveWorkspaceID, isFocused],

What alternative solutions did you explore? (Optional)

• Instead of using Navigation.goBack();, we can use Navigation.dismissModal();. https://github.com/Expensify/App/blob/81e877a3e75e51fc19a14e43e4632d3889949584/src/pages/WorkspaceSwitcherPage/index.tsx#L94

Result

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[s77rt]

@nyomanjyotisa Thanks for the proposal. Do you know why this happens on mWeb Chrome only?

[s77rt]

@Krishna2323 Thanks for the proposal. Same question ^ any idea why this does not happen on other platforms?

[Krishna2323]

@s77rt, It also happens on mWeb Safari, though I don’t have a clear understanding of why it behaves differently. I think this is the default behavior of mobile web, and it totally depends on how quickly the pressables are removed from the screen after selecting an option. I have also added an alternative solution in my proposal, please let me know your thoughts on that.

https://github.com/user-attachments/assets/4ea1bbef-61ff-48bb-aaaa-d97d25de2f90

[s77rt]

@Krishna2323 Thanks for the update. I think using dismissModal makes more sense. Can you please check if this works as expected when using the browser's back button? Also what will happen on the second execution of that function? (Let's double check this does not cause unwanted side effects)

[s77rt]

@Krishna2323 Did dimissModal work for you? From my testing I'm still getting the same bug

[Krishna2323]

@s77rt, works perfectly on my machine.

https://github.com/user-attachments/assets/72d0b8ea-2405-4d92-85e2-bc0ad1e21b52

[Krishna2323]

Also what will happen on the second execution of that function? (Let's double check this does not cause unwanted side effects)

• I think a more better way is to just call Navigation.dismissModal(); and return if policyID === activeWorkspaceID. What do you think. Updating Navigation.goBack(); to Navigation.dismissModal(); is also safe as per my testing.
• We can also remove Navigation.goBack(); because the navigation will be handled by Navigation.navigateWithSwitchPolicyID({policyID}); in case policyID !== activeWorkspaceID is true.

    const selectPolicy = useCallback(
        (option?: WorkspaceListItem) => {
            if (!option) {
                return;
            }

            const {policyID} = option;
            if (policyID === activeWorkspaceID) {
                Navigation.dismissModal();
                return;
            }

            setActiveWorkspaceID(policyID);
            Navigation.goBack();
            if (policyID !== activeWorkspaceID) {
                Navigation.navigateWithSwitchPolicyID({policyID});
            }
        },
        [activeWorkspaceID, setActiveWorkspaceID],
    );

[s77rt]

@Krishna2323 I think we still have another problem. After the first click the activeWorkspaceID is changed and so the position of the selected workspace. Now the second click is made on the previous workspace and causes same bug.

https://github.com/user-attachments/assets/7f87f157-7c9d-4798-8ae6-92f4f2b61ae3

First click:

Second click:

[s77rt]

https://github.com/user-attachments/assets/4ca20fe9-1673-4317-bf75-d8b6fdc06120

[Krishna2323]

@s77rt, what do you think about using isFocused state? isFocused is used in multiple places to fix similar bugs.

https://github.com/Expensify/App/blob/d78be88cf3366d90160ef924e66bc4834e6e29a8/src/pages/home/report/ReportActionCompose/AttachmentPickerWithMenuItems.tsx#L296-L301

https://github.com/Expensify/App/blob/d78be88cf3366d90160ef924e66bc4834e6e29a8/src/pages/workspace/WorkspaceMembersPage.tsx#L151-L159

[s77rt]

@Krishna2323 I think that would be a workaround because the selectPolicy callback is not really expected to be called if the screen is not focused. I'm not sure how such case can be handled but let's try explore more options for now. Do you know if this is a new bug? or if we the bug exist on other places?

[melvin-bot[bot]]

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

[s77rt]

Still looking for proposals

[Krishna2323]

@s77rt, I found this bug which is very similar to ours and was solved using isFocused state.

[s77rt]

@Krishna2323 The problem is that we'd be only solving a case instead of solving the bug completely. Can we maybe have this in GenericPressable? (We have useSingleExecution that seems to solve a similar issue on native)

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

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

[s77rt]

Still looking for proposals

[FitseTLT]

Edited by proposal-police: This proposal was edited at 2024-11-05 19:16:25 UTC.

Proposal

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

mWeb - Selector - User lands on the same WS when double tapping on different one on selector

What is the root cause of that problem?

We are calling selectPolicy twice here https://github.com/Expensify/App/blob/210db71f7e549abf7c6b925e36029549ece83256/src/pages/WorkspaceSwitcherPage/index.tsx#L85-L100 but the second execution sets the policy back to the first

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

We need to use shouldSingleExecuteRowSelect here https://github.com/Expensify/App/blob/210db71f7e549abf7c6b925e36029549ece83256/src/pages/WorkspaceSwitcherPage/index.tsx#L190

                        shouldSingleExecuteRowSelect

And currently our useSingleExecution hook doesn't work for web but we need the update to apply the single execution for mobile

import {useCallback} from 'react';
import * as Browser from '@libs/Browser';
import useSingleExecutionMobile from './index.native';

type Action<T extends unknown[]> = (...params: T) => void | Promise<void>;

/**
 * This hook was specifically written for native issue
 * more information: https://github.com/Expensify/App/pull/24614 https://github.com/Expensify/App/pull/24173
 * on web we don't need this mechanism so we just call the action directly.
 */
const isMobile = Browser.isMobile();

const useSingleExecutionRest = () => {
    const singleExecution = useCallback(
        <T extends unknown[]>(action?: Action<T>) =>
            (...params: T) => {
                action?.(...params);
            },
        [],
    );

    return {isExecuting: false, singleExecution};
};
const useSingleExecution = isMobile ? useSingleExecutionMobile : useSingleExecutionRest;
export default useSingleExecution;

export type {Action};

We can also make it take a param that we will set on for this case only and apply the singleExecution when the param is true but I think there is nothing we will lose if we apply the single execution in all other instances of the app

What alternative solutions did you explore? (Optional)

We can also make useSingleExecution apply to all platforms by moving the code in .native to the index file

[s77rt]

@FitseTLT Thanks for the proposal. Your RCA is correct. Can you please explain the logic of useSingleExecution on mWeb (or in web in general)?

[FitseTLT]

@FitseTLT Thanks for the proposal. Your RCA is correct. Can you please explain the logic of useSingleExecution on mWeb (or in web in general)?

The logic of useSingleExecution is to return a callback that will call the callback you pass to it and it will set an internal state of isExecuting that once it runs the action you passed and will only be reset after InteractionManager.runAfterInteraction to avoid the repeated calls before the interaction is finished as it will no re-run the action when isExecuting is true. But now it is only being used in native, so I am proposing to apply it for mWeb or for all platforms, as I think we would lose nothing.

[s77rt]

@FitseTLT I'm afraid that InteractionManager.runAfterInteraction may cause a significant delay making the app look slow. Can you please test this approach?

[FitseTLT]

@FitseTLT I'm afraid that InteractionManager.runAfterInteraction may cause a significant delay making the app look slow. Can you please test this approach?

@s77rt Currently, everywhere we are using shouldSingleExecuteRowSelect we are already doing that for native, so we can't expect the onSelectRow to be called multiple times before the interaction is finished for native. Therefore, applying the same effect on mWeb on those instances can not cause unnecessary lag because there could not be a use case where we want the onSelectRow to be called multiple times in mWeb but to not be called in native, hence, we are safe to apply the changes. However if we have any doubt we can even have a new param, may be shouldSingleExecuteInmWeb, and an apply the single execution in mweb in that case only, but, for the reason I mention above, there is no need to do it. We can safely apply the changes for mWeb; we can even be solving similar problem in the code base that are occurring in mWeb.