[$500] [Ideal Nav] Chats - Breadcrumbs shows different workspace when another workspace is selected via keyboard

[lanitochka17]

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: 1.4.52-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: N/A Issue reported by: Applause - Internal Team

Action Performed:

Precondition:

• User has a few workspaces

1. Go to staging.new.expensify.com
2. Open workspace switcher
3. Open a different workspace using keyboard arrow keys and Enter

Expected Result:

The breadcrumbs will show the correct workspace

Actual Result:

The breadcrumbs shows different workspace when another workspace is selected via keyboard

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/cbc2f374-9f18-48d3-8138-3d555ef8b110

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0165d93da7594b5802
• Upwork Job ID: 1769822686303518720
• Last Price Increase: 2024-03-25

[melvin-bot[bot]]

Triggered auto assignment to @dylanexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[lanitochka17]

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

[lanitochka17]

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

[bernhardoj]

Proposal

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

The breadcrumb shows the previous selected workspace when selecting it with keyboard.

What is the root cause of that problem?

When we select a workspace, it will do 3 things, set the new active workspace ID, navigate back, and push a new bottom tab screen with a new policy ID params. https://github.com/Expensify/App/blob/0a512c9f3cf1b207668ab93f3d79866c80345f88/src/pages/WorkspaceSwitcherPage.tsx#L113-L117

Then, we also have a nav state change listener that will also set the new active workspace ID based on the top most bottom tab policy ID. https://github.com/Expensify/App/blob/0a512c9f3cf1b207668ab93f3d79866c80345f88/src/libs/Navigation/NavigationRoot.tsx#L115-L124

The issue we have here is the state listener above is called with an undefined policy ID. It's undefined when we call Navigation.goBack because, at this point, there is no policy ID yet in the params. After the switch policy ID happens, we get the correct policy ID. Weirdly, if we select it normally, the navigation goBack and switch policy ID happens in a batch, so we never get an undefined policy ID.

The next problem is because the active workspace ID is updated very quickly between the correct policy ID and undefined, withOnyx can't give the correct onyx data. This is a bug in onyx itself. You can test it by having a button that will switch the active workspace ID between an undefined and a real policy ID and you will see the issue.

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

Maybe we should fix the onyx issue, but I would like to propose to update the active workspace ID only when the nav state changes. In short, we will remove the setActiveWorkspaceID here. https://github.com/Expensify/App/blob/0a512c9f3cf1b207668ab93f3d79866c80345f88/src/pages/WorkspaceSwitcherPage.tsx#L113

and keep what we have here. https://github.com/Expensify/App/blob/0a512c9f3cf1b207668ab93f3d79866c80345f88/src/libs/Navigation/NavigationRoot.tsx#L115-L124

[dragnoir]

Proposal

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

Breadcrumbs shows different workspace when another workspace is selected via keyboard

What is the root cause of that problem?

The underlying cause is the disparate handling of keyboard and mouse events. Currently, there is a distinct listener and handler for each event type.

when we click on a workspace from the workspace switcher list, this function below is triggered: https://github.com/Expensify/App/blob/4e599b2cd6061b52d3964dc69e7575e210f0581d/src/pages/WorkspaceSwitcherPage.tsx#L100-L120 However, the use of useContext within the setActiveWorkspaceID(policyID); function may cause execution delays for subsequent functions.

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

If we remove setActiveWorkspaceID(policyID);, then there will be a lagg and delay when displaying the workspace in the Breadcrumbs when we navigating back.

The solution involves reordering the function calls within the selectPolicy method to optimize the sequence of operations. Specifically, the call to setActiveWorkspaceID(policyID); should be moved to execute after the navigation actions have completed. This change aims to synchronize the breadcrumb display with the selected workspace, eliminating the delay.

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

            const {policyID} = option;

            if (policyID) {
                setSelectedOption(option);
            } else {
                setSelectedOption(undefined);
            }
-           setActiveWorkspaceID(policyID);
            Navigation.goBack();
            if (policyID !== activeWorkspaceID) {
                Navigation.navigateWithSwitchPolicyID({policyID});
            }
+           setActiveWorkspaceID(policyID);
        },
        [activeWorkspaceID, setActiveWorkspaceID],
    );

What alternative solutions did you explore? (Optional)

Due that we have two navigations, the goBack and the Navigation.navigateWithSwitchPolicyID.

We can remove the goBack, that way we make sure that setActiveWorkspaceID(policyID); execute than the page switch happens.

const selectPolicy = useCallback(

            setActiveWorkspaceID(policyID);

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

[trjExpensify]

Isn't this the same? https://github.com/Expensify/App/issues/38281

[dragnoir]

Isn't this the same? #38281

Not the same.

[trjExpensify]

Got it, I thought they might be related or the solution here fixes the delay.

[dragnoir]

I tested the solution but I still see the issue. It's different.

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[dylanexpensify]

Yep, repro'd. Nice catch!

[dragnoir]

@rushatgabhane just for info, my alternative solution tested with the best performance, no glitch when the workspace is selected, same behavior as a mouse click.

[dylanexpensify]

bump @rushatgabhane

[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? 💸

[trjExpensify]

@rushatgabhane can you look at this one, please?

[dylanexpensify]

Pending @rushatgabhane review

[rushatgabhane]

yes gimme a moment

[rushatgabhane]

I like @bernhardoj's proposal to update the active workspace ID only when the nav state changes.

🎀 👀 🎀

[melvin-bot[bot]]

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

[dragnoir]

@rushatgabhane when you remove the setActiveWorkspaceID, as the proposal selected. The UI will have a glitch and the workspace icon will not be displayed in time.

Pls check video:

https://github.com/Expensify/App/assets/12425932/81003249-e187-4e89-bb30-31508e9ba2df

We need to keep setActiveWorkspaceID

[rushatgabhane]

The UI will have a glitch and the workspace icon will not be displayed in time

I think that looks alright. The avatar will take some time to update

[dragnoir]

@rushatgabhane, I believe the current approach might not be optimal for both user experience and performance.

In my proposed solution, the transition between workspaces is seamless, allowing the new avatar to appear instantly. It's crucial to maintain setActiveWorkspaceID to ensure this efficiency.

I've also conducted tests on a lower-end computer, and the difference in response time was significantly noticeable.

Please check this video with a 2x slowdown CPU (the proposal you picked)

https://github.com/Expensify/App/assets/12425932/93e9b67d-c161-4ff8-ab53-6d1dc5e8d549

and please check with my solution with the same 2x slowdown CPU

https://github.com/Expensify/App/assets/12425932/7a7edf39-fa91-4322-9008-c04223cc8396

I want to emphasize the importance of maintaining high-quality code standards. It's crucial not just for immediate functionality but for the long-term maintainability and scalability of the project. Ensuring our code is clean and efficient helps prevent technical debt and facilitates easier updates and debugging in the future.

Why do we need to go with a bad solution if we have a better one?.

[dragnoir]

@rushatgabhane pls check my comment https://github.com/Expensify/App/issues/38297#issuecomment-2022463402 when you have time

[bernhardoj]

We always need to call goBack to close the WS switcher page. I'm okay if you want to go with @dragnoir main solution if it works well.

[rushatgabhane]

@dragnoir thanks for the explaination. I like your solution 🎀 👀 🎀

https://github.com/Expensify/App/issues/38297#issuecomment-1999220296

[melvin-bot[bot]]

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

[melvin-bot[bot]]

📣 @dragnoir You have been assigned to this job! Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs! Keep in mind: Code of Conduct | Contributing 📖

[dragnoir]

PR ready https://github.com/Expensify/App/pull/39787

[trjExpensify]

PR merged, nice!