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

IuliiaHerets commented 1 month ago

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: 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:

Open the staging.new.expensify.com website.
Tap on Workspace selector on the top left corner.
Change to any workspace on the list.
Double tap on Expensify option.
Verify that all the chats are displayed in LHN.
Open Workspace selector again.
Double tap on the next workspace to the one selected.
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] commented 1 month ago

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 commented 1 month ago

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

IuliiaHerets commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

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] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

s77rt commented 1 month ago

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

s77rt commented 1 month ago

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

Krishna2323 commented 1 month ago

@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 commented 1 month ago

@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 commented 1 month ago

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

Krishna2323 commented 1 month ago

@s77rt, works perfectly on my machine.

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

Krishna2323 commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

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

Krishna2323 commented 1 month ago

@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 commented 1 month ago

@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] commented 1 month ago

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

s77rt commented 1 month ago

Still looking for proposals

Krishna2323 commented 1 month ago

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

s77rt commented 1 month ago

@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] commented 1 month ago

📣 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] commented 4 weeks ago

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

s77rt commented 4 weeks ago

Still looking for proposals

FitseTLT commented 4 weeks ago

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 commented 4 weeks ago

@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 commented 4 weeks ago

@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 commented 4 weeks ago

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

FitseTLT commented 3 weeks ago

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

s77rt commented 3 weeks ago

@FitseTLT The implementation of InteractionManager.runAfterInteraction is different in web and it will always create a promise (async run) even if there is no interaction going on. There may be a reason why it was not implemented in web at first. It's better to at least run some tests

FitseTLT commented 3 weeks ago

Okay @s77rt Here are test videos for select policy and one other place where we use it: room visibility setting page and it works perfect. Still, if you doubt InteractionManager.runAfterInteraction for web we can use setTimeout for mWeb 👍. Android Web:

https://github.com/user-attachments/assets/bcc18596-bfb8-418b-a491-c80fe07209a1

IOS Web:

https://github.com/user-attachments/assets/b295e5cc-560c-4b04-a43d-5fd87aa40338

Desktop:

https://github.com/user-attachments/assets/7becec67-50f5-4c8f-ba3c-5f39acd96cee

Web:

https://github.com/user-attachments/assets/a8bffc31-5906-4deb-abfe-a4d2484d0853

s77rt commented 3 weeks ago

@FitseTLT I found that it useSingleExecution was implemented in web but removed https://github.com/Expensify/App/commit/0551c8b0ea8311987b38f2be8d70a0ac3f9a6d30 cc @getusha maybe you recall why it had to be removed or if it caused any bugs?

FitseTLT commented 3 weeks ago

@FitseTLT I found that it useSingleExecution was implemented in web but removed 0551c8b cc @getusha maybe you recall why it had to be removed or if it caused any bugs?

I am also thinking of using setTimeout for mWeb as we can't be sure about the behaviour in different browsers according to https://github.com/necolas/react-native-web/discussions/1943 . WDYT @s77rt

s77rt commented 3 weeks ago

@FitseTLT That discussion is outdated and no longer true, requestIdleCallback and setTimeout are no longer used.

getusha commented 3 weeks ago

@getusha maybe you recall why it had to be removed or if it caused any bugs?

@s77rt not sure but it's probably to fix https://github.com/Expensify/App/pull/24482#issuecomment-1748498453 https://github.com/Expensify/App/pull/24482#issuecomment-1748632679

s77rt commented 3 weeks ago

@getusha Thank you! @FitseTLT Would you be able to check against the linked bugs?

melvin-bot[bot] commented 3 weeks ago

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

Krishna2323 commented 3 weeks ago

@s77rt, I still think this isn't a web bug, rather, it's the default behavior of the web. You can click on multiple links until the page is completely removed.

We are facing this specific issue in WorkspaceSwitcherPage due to the logic in selectPolicy, which calls Navigation.goBack(); and Navigation.navigateWithSwitchPolicyID({policyID}); together. In my opinion, this is necessary. We could use the isFocused state for this specific case, as I don't think this issue will occur everywhere. We are already using isFocused to solve similar cases.

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

FitseTLT commented 3 weeks ago

@getusha Thank you! @FitseTLT Would you be able to check against the linked bugs?

Yep checked the comments @s77rt , especially https://github.com/Expensify/App/pull/24482#issuecomment-1748498453 looks more relevant (User should be able to change Select all - Deselect all fast and smoothly. There should be no lagging) but the problem was on web but on mWeb it is more or less as native, in terms of the speed and smoothness of the app, as it isn't as fast desktop/web so applying the singleExecution on isMobile only is still feasible I think and it tests well.

melvin-bot[bot] commented 3 weeks ago

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

s77rt commented 3 weeks ago

@Krishna2323 I see your point. But the fact that this is only reproducible on mWeb is still a concern. Can you please check if this is reproducible in native?

s77rt commented 3 weeks ago

@FitseTLT I don't think applying useSingleExecution just on mWeb is worth it. It will also create more inconsistency

Krishna2323 commented 3 weeks ago

Can you please check if this is reproducible in native?

Because of singleExecution on native, it is not reproducible. But if we remove the singleExecution, we can see the same bug on native too.

https://github.com/user-attachments/assets/fcb4f134-3585-4a70-a0a9-fafa7e68d0d9

FitseTLT commented 3 weeks ago

@FitseTLT I don't think applying useSingleExecution just on mWeb is worth it. It will also create more inconsistency

@s77rt I honestly can't see the inconsistency. The App speed in mWeb is more less the same as the native and this kind of issue can occur for the future on mWeb if the user taps multiple times too quick. So I think we should apply the same singleExecution solution we used in native. And that will avoid unnecessary lagging in the app for web/desktop as singleExecution is not needed for those platforms.

s77rt commented 3 weeks ago

@Krishna2323 I think we can proceed with the focus hook for now. We can revisit this if similar bugs occurs.

🎀 👀 🎀 C+ reviewed Link to proposal

s77rt commented 3 weeks ago

@FitseTLT The inconsistency is that we'd use InteractionManager on mWeb but not on Web even though they share the same underlaying logic.

melvin-bot[bot] commented 3 weeks ago

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

blimpich commented 3 weeks ago

@Krishna2323 @s77rt If possible I'd like us to see if we can write a unit test along with this fix in order to prevent this from regressing in the future.

melvin-bot[bot] commented 3 weeks ago

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

Expensify / App