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

IuliiaHerets · 2024-10-24T11:46:40Z

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:

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:

Screenshots/Videos

Bug6643981_1729740526844.Selector_WS.mp4

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 · 2024-10-24T11:46:49Z

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 · 2024-10-24T11:47:39Z

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

IuliiaHerets · 2024-10-24T11:48:02Z

@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 · 2024-10-24T12:02:08Z

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

App/src/pages/WorkspaceSwitcherPage/index.tsx

Lines 85 to 100 in 09f19c3

    
           const selectPolicy = useCallback( 
        
               (option?: WorkspaceListItem) => { 
        
                   if (!option) { 
        
                       return; 
        
                   } 
        
                   const {policyID} = option; 
        
                   setActiveWorkspaceID(policyID); 
        
                   Navigation.goBack(); 
        
                   if (policyID !== activeWorkspaceID) { 
        
                       Navigation.navigateWithSwitchPolicyID({policyID}); 
        
                   } 
        
               }, 
        
               [activeWorkspaceID, setActiveWorkspaceID], 
        
           );

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 · 2024-10-24T13:37:40Z

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.

App/src/pages/WorkspaceSwitcherPage/index.tsx

Lines 85 to 100 in 81e877a

    
           const selectPolicy = useCallback( 
        
               (option?: WorkspaceListItem) => { 
        
                   if (!option) { 
        
                       return; 
        
                   } 
        
                   const {policyID} = option; 
        
                   setActiveWorkspaceID(policyID); 
        
                   Navigation.goBack(); 
        
                   if (policyID !== activeWorkspaceID) { 
        
                       Navigation.navigateWithSwitchPolicyID({policyID}); 
        
                   } 
        
               }, 
        
               [activeWorkspaceID, setActiveWorkspaceID], 
        
           );

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();.

App/src/pages/WorkspaceSwitcherPage/index.tsx

Line 94 in 81e877a

Navigation.goBack();

Result

melvin-bot · 2024-10-26T00:49:01Z

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

melvin-bot · 2024-10-26T00:49:06Z

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

s77rt · 2024-10-26T03:54:46Z

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

s77rt · 2024-10-26T03:55:27Z

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

Krishna2323 · 2024-10-27T21:59:15Z

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

bug_ios_safari_double_tap.mp4

s77rt · 2024-10-28T10:36:07Z

@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 · 2024-10-28T21:45:22Z

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

Krishna2323 · 2024-10-28T23:52:30Z

@s77rt, works perfectly on my machine.

double_navigation_workspace_switcher.mp4

Krishna2323 · 2024-10-29T00:04:54Z

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 · 2024-10-29T08:17:18Z

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

Screen.Recording.2024-10-29.at.9.04.05.AM.mov

First click:

Second click:

s77rt · 2024-10-29T08:18:23Z

Screen.Recording.2024-10-29.at.9.16.15.AM.mov

Krishna2323 · 2024-10-29T14:07:46Z

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

App/src/pages/home/report/ReportActionCompose/AttachmentPickerWithMenuItems.tsx

Lines 296 to 301 in d78be88

    
           onPress={(e) => { 
        
               e?.preventDefault(); 
        
               if (!isFocused) { 
        
                   return; 
        
               } 
        
               onAddActionPressed();

App/src/pages/workspace/WorkspaceMembersPage.tsx

Lines 151 to 159 in d78be88

    
           // useFocus would make getWorkspaceMembers get called twice on fresh login because policyEmployee is a dependency of getWorkspaceMembers. 
        
           useEffect(() => { 
        
               if (!isFocused) { 
        
                   setSelectedEmployees([]); 
        
                   return; 
        
               } 
        
               getWorkspaceMembers(); 
        
               // eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps 
        
           }, [isFocused]);

s77rt · 2024-10-29T16:53:51Z

@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 · 2024-11-01T18:19:33Z

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

s77rt · 2024-11-01T18:26:40Z

Still looking for proposals

Krishna2323 · 2024-11-02T00:31:33Z

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

FitseTLT · 2024-11-07T14:08:46Z

@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 necolas/react-native-web#1943 . WDYT @s77rt

s77rt · 2024-11-07T15:44:11Z

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

getusha · 2024-11-07T15:52:11Z

@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 #24482 (comment) #24482 (comment)

s77rt · 2024-11-07T17:14:30Z

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

melvin-bot · 2024-11-09T16:00:05Z

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

Krishna2323 · 2024-11-11T06:16:12Z

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

App/src/pages/WorkspaceSwitcherPage/index.tsx

Lines 85 to 100 in 81e877a

    
           const selectPolicy = useCallback( 
        
               (option?: WorkspaceListItem) => { 
        
                   if (!option) { 
        
                       return; 
        
                   } 
        
                   const {policyID} = option; 
        
                   setActiveWorkspaceID(policyID); 
        
                   Navigation.goBack(); 
        
                   if (policyID !== activeWorkspaceID) { 
        
                       Navigation.navigateWithSwitchPolicyID({policyID}); 
        
                   } 
        
               }, 
        
               [activeWorkspaceID, setActiveWorkspaceID], 
        
           );

FitseTLT · 2024-11-11T10:39:31Z

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

Yep checked the comments @s77rt , especially #24482 (comment) 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 · 2024-11-11T15:21:56Z

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

s77rt · 2024-11-11T17:54:04Z

@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 · 2024-11-11T17:54:56Z

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

Krishna2323 · 2024-11-11T21:25:31Z

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.

Monosnap.screencast.2024-11-12.02-53-23.mp4

FitseTLT · 2024-11-12T13:34:32Z

@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 · 2024-11-12T15:46:22Z

@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 · 2024-11-12T15:46:26Z

@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 · 2024-11-12T15:46:36Z

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

blimpich · 2024-11-12T18:00:16Z

@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 · 2024-11-12T18:00:35Z

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

stephanieelliott · 2024-11-19T06:49:48Z

PR is in review

stephanieelliott · 2024-11-26T05:30:23Z

PR is moving along, @Krishna2323 is working on some tests

IuliiaHerets added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Oct 24, 2024

melvin-bot bot assigned stephanieelliott Oct 24, 2024

stephanieelliott added the External Added to denote the issue can be worked on by a contributor label Oct 26, 2024

melvin-bot bot changed the title ~~mWeb - Selector - User lands on the same WS when double tapping on different one on selector~~ [$250] mWeb - Selector - User lands on the same WS when double tapping on different one on selector Oct 26, 2024

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 26, 2024

melvin-bot bot assigned s77rt Oct 26, 2024

melvin-bot bot added the Overdue label Nov 1, 2024

melvin-bot bot removed the Overdue label Nov 1, 2024

melvin-bot bot added the Overdue label Nov 11, 2024

melvin-bot bot removed the Overdue label Nov 11, 2024

melvin-bot bot assigned blimpich Nov 12, 2024

blimpich assigned Krishna2323 Nov 12, 2024

melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 12, 2024

stephanieelliott added this to [#whatsnext] #expense Nov 12, 2024

Krishna2323 mentioned this issue Nov 13, 2024

fix: mWeb - Selector - User lands on the same WS when double tapping on different one on selector. #52438

Open

49 tasks

melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Nov 13, 2024

garrettmknight moved this to Bugs and Follow Up Issues in [#whatsnext] #expense Nov 14, 2024

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

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

Comments

IuliiaHerets commented Oct 24, 2024 • edited by melvin-bot bot Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

melvin-bot bot commented Oct 24, 2024

IuliiaHerets commented Oct 24, 2024

IuliiaHerets commented Oct 24, 2024

nyomanjyotisa commented Oct 24, 2024 • edited Loading

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

Krishna2323 commented Oct 24, 2024 • edited by github-actions bot Loading

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

Result

melvin-bot bot commented Oct 26, 2024

melvin-bot bot commented Oct 26, 2024

s77rt commented Oct 26, 2024

s77rt commented Oct 26, 2024

Krishna2323 commented Oct 27, 2024 • edited Loading

s77rt commented Oct 28, 2024

s77rt commented Oct 28, 2024

Krishna2323 commented Oct 28, 2024

Krishna2323 commented Oct 29, 2024 • edited Loading

s77rt commented Oct 29, 2024

s77rt commented Oct 29, 2024

Krishna2323 commented Oct 29, 2024

s77rt commented Oct 29, 2024

melvin-bot bot commented Nov 1, 2024

s77rt commented Nov 1, 2024

Krishna2323 commented Nov 2, 2024

FitseTLT commented Nov 7, 2024

s77rt commented Nov 7, 2024

getusha commented Nov 7, 2024

s77rt commented Nov 7, 2024

melvin-bot bot commented Nov 9, 2024

Krishna2323 commented Nov 11, 2024 • edited Loading

FitseTLT commented Nov 11, 2024 • edited Loading

melvin-bot bot commented Nov 11, 2024

s77rt commented Nov 11, 2024

s77rt commented Nov 11, 2024

Krishna2323 commented Nov 11, 2024

FitseTLT commented Nov 12, 2024 • edited Loading

s77rt commented Nov 12, 2024

s77rt commented Nov 12, 2024

melvin-bot bot commented Nov 12, 2024

blimpich commented Nov 12, 2024

melvin-bot bot commented Nov 12, 2024

stephanieelliott commented Nov 19, 2024

stephanieelliott commented Nov 26, 2024

IuliiaHerets commented Oct 24, 2024 •

edited by melvin-bot bot

Loading

nyomanjyotisa commented Oct 24, 2024 •

edited

Loading

Krishna2323 commented Oct 24, 2024 •

edited by github-actions bot

Loading

Proposal 

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

Krishna2323 commented Oct 27, 2024 •

edited

Loading

Krishna2323 commented Oct 29, 2024 •

edited

Loading

Krishna2323 commented Nov 11, 2024 •

edited

Loading

FitseTLT commented Nov 11, 2024 •

edited

Loading

FitseTLT commented Nov 12, 2024 •

edited

Loading