[$500] Workspace - Web - Unable to go to Workspace setting when open member chat page

[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.58-0 Reproducible in staging?: Y Reproducible in production?: N If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Email or phone of affected tester (no customers): [email protected] Issue reported by: Applause - Internal Team

Action Performed:

1. Go to Profile> Workspaces> Create Workspace
2. Go to Members> Invite member> Invite any email address
3. Click on invited member section to open Profile page
4. Select message user
5. Go to Go to Profile> Workspaces
6. Click on the workspace to open settings page

Expected Result:

Workspace settings page should open

Actual Result:

Workspace settings page can not be open, nothing happens when click on workspace

Workaround:

Unknown

Platforms:

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

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/116fbd07-5100-4ff1-9710-f12df1c31e41

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~017de9a21211d600e9
• Upwork Job ID: 1773798612438458368
• Last Price Increase: 2024-03-29

[melvin-bot[bot]]

Triggered auto assignment to @thienlnam (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

[github-actions[bot]]

:wave: Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[lanitochka17]

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

[thienlnam]

Looks like it could be external, not entirely sure if this is expected or not though

[thienlnam]

Actually, it does seem to be unintended especially since you are the policy owner

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[shahinyan11]

Proposal

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

Workspace - Web - Unable to go to Workspace setting when open member chat page

What is the root cause of that problem?

The Workspace setting page is a FULL_SCREEN_NAVIGATOR screen. When we navigation to a full screen navigator screens without second param in Navigation.navigate ( in our case from here ) the code goes into this else if block. And in our case, the getPartialStateDiff function does not find differences and will return on this line without any navigation actions.

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

Update this condition like below

if (
    (!stateTopmostFullScreen && templateStateTopmostFullScreen) ||
    (stateTopmostFullScreen &&
        templateStateTopmostFullScreen &&
        (stateTopmostFullScreen.name !== templateStateTopmostFullScreen.name || !shallowCompare(stateTopmostFullScreen.params, templateStateTopmostFullScreen.params)))
) 

What alternative solutions did you explore? (Optional)

Add type param as CONST.NAVIGATION.TYPE.UP for navigation here

[ahmedGaber93]

Proposal

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

Workspace - Web - Unable to go to Workspace setting when open member chat page

What is the root cause of that problem?

The workspace screen navigate work by comparing differences between rootState and adaptedState https://github.com/Expensify/App/blob/b5797f91aa0f26c8d1669bf3ed0a4e1a7ac2514b/src/libs/Navigation/linkTo.ts#L199-L202

and dispatch the diff here https://github.com/Expensify/App/blob/b5797f91aa0f26c8d1669bf3ed0a4e1a7ac2514b/src/libs/Navigation/linkTo.ts#L195-L197

the issue here when we calculate the diff here, the diff it only set if no any NAVIGATORS.FULL_SCREEN_NAVIGATOR screen in the history, and in our case we already have, so no diff dispatch here and the code return early here without any action. https://github.com/Expensify/App/blob/b5797f91aa0f26c8d1669bf3ed0a4e1a7ac2514b/src/libs/Navigation/AppNavigator/getPartialStateDiff.ts#L78-L80

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

We need to calculate the diff correctly.

1. create new method getTopmostFullScreen like getTopmostCentralPaneRoute instead of this short calculation

2. Use the similar logic here instead of this

this fast change to test

// we will handle this by create new method `getTopmostFullScreen` like `getTopmostCentralPaneRoute`
// for test comment this line and ad the below instead
// const stateTopmostFullScreen = state.routes.filter((route) => route.name === NAVIGATORS.FULL_SCREEN_NAVIGATOR).at(-1);

let stateTopmostFullScreen = state.routes.filter((route) => route.name === NAVIGATORS.FULL_SCREEN_NAVIGATOR).at(-1);
if (!!stateTopmostFullScreen?.params && 'screen' in stateTopmostFullScreen.params) {
    stateTopmostFullScreen = {name: stateTopmostFullScreen.params.screen as CentralPaneName, params: stateTopmostFullScreen.params.params};
}

// the condition will be
if ((!stateTopmostFullScreen && templateStateTopmostFullScreen) ||
  (stateTopmostFullScreen &&
      templateStateTopmostFullScreen &&
      stateTopmostFullScreen.name !== templateStateTopmostFullScreen.name) &&
  !shallowCompare(stateTopmostFullScreen.params, templateStateTopmostFullScreen.params)
)

What alternative solutions did you explore? (Optional)

we can also don't return here when there is no diff.

let diffActions = [];
if (adaptedState && (metainfo.isCentralPaneAndBottomTabMandatory || metainfo.isFullScreenNavigatorMandatory)) {
  diffActions = ...
}

// don't return early if no diff dispatched.
// if (action.payload.name === NAVIGATORS.FULL_SCREEN_NAVIGATOR && !isNarrowLayout) {
if (diffActions.length > 0 && action.payload.name === NAVIGATORS.FULL_SCREEN_NAVIGATOR && !isNarrowLayout) {
    return;
}

[shahinyan11]

Proposal

Updated. Add more detailed description for root cause and new solution as main

[ahmedGaber93]

@Ollyws just for confirmation, please don't forget to review the edited history for proposals, as I think my proposal is the first correct root cause. Thanks!

[ikevin127]

Proposal

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

Unable to navigate to Workspace Settings after opening member chat page.

What is the root cause of that problem?

The root cause of the problem lies in the conditional logic within this code block.

The problem is that the current logic for detecting a difference in FULL_SCREEN_NAVIGATOR assumes there is a difference only if the FULL_SCREEN_NAVIGATOR was not previously navigated to (!stateTopmostFullScreen). This means if stateTopmostFullScreen exists, it will never return a difference - hence we're not navigated to the Workspace Settings in our case.

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

To resolve this issue, the condition within the code block should be extended to also check if the topmost full screen route in the templateState differs from the one in the current state, similar to what we do above in the if (metainfo.isCentralPaneAndBottomTabMandatory) block.

We should compare whether the route names or the parameters of the two topmost full screen routes are different, which would then mean there is a change that needs to be accounted for in the diff and we will be navigated to the Workspace Settings as expected.

Code changes (diff)

    if (metainfo.isFullScreenNavigatorMandatory) {
        const stateTopmostFullScreen = state.routes.filter((route) => route.name === NAVIGATORS.FULL_SCREEN_NAVIGATOR).at(-1);
        const templateStateTopmostFullScreen = templateState.routes.filter((route) => route.name === NAVIGATORS.FULL_SCREEN_NAVIGATOR).at(-1) as NavigationPartialRoute;

-       if (!stateTopmostFullScreen && templateStateTopmostFullScreen) {

+       // Check if there is no topmost full screen route in the current state or if the route names or params are different
+       if ((!stateTopmostFullScreen && templateStateTopmostFullScreen) || 
+           (stateTopmostFullScreen && templateStateTopmostFullScreen &&
+               (stateTopmostFullScreen.name !== templateStateTopmostFullScreen.name ||
+               !shallowCompare(stateTopmostFullScreen.params, templateStateTopmostFullScreen.params)))) {
            diff[NAVIGATORS.FULL_SCREEN_NAVIGATOR] = templateStateTopmostFullScreen;
        }
    }

Videos (before / after)

MacOS: Chrome / Safari

Before	After

[Ollyws]

@ahmedGaber93 Your RCA seems correct but I can't get your example to work because !shallowCompare(stateTopmostFullScreen.params, templateStateTopmostFullScreen.params) is returning true when I follow the reproduction steps.

[ahmedGaber93]

@Ollyws Hmm. I think you may missed something, it works fine with me. Here is the full code for (just quick code for test)

    if (metainfo.isFullScreenNavigatorMandatory) {
        let stateTopmostFullScreen = state.routes.filter((route) => route.name === NAVIGATORS.FULL_SCREEN_NAVIGATOR).at(-1);
        if (!!stateTopmostFullScreen?.params && 'screen' in stateTopmostFullScreen.params) {
            stateTopmostFullScreen = {name: stateTopmostFullScreen.params.screen, params: stateTopmostFullScreen.params.params};
        }
        const templateStateTopmostFullScreen = templateState.routes.filter((route) => route.name === NAVIGATORS.FULL_SCREEN_NAVIGATOR).at(-1) as NavigationPartialRoute;
        
        if ((!stateTopmostFullScreen && templateStateTopmostFullScreen) ||
            (stateTopmostFullScreen &&
                templateStateTopmostFullScreen &&
                stateTopmostFullScreen.name !== templateStateTopmostFullScreen.name) &&
            !shallowCompare(stateTopmostFullScreen.params, templateStateTopmostFullScreen.params)
        ) {
            diff[NAVIGATORS.FULL_SCREEN_NAVIGATOR] = templateStateTopmostFullScreen;
        }
    }

result

https://github.com/Expensify/App/assets/41129870/6c7c4fd5-50d6-4be4-b54d-c574c3763a10

[ahmedGaber93]

@Ollyws the code is work fine, but I found error in my logic, templateStateTopmostFullScreen not have params attribute.

{
  "name": "FullScreenNavigator",
  "state": {
    "routes": [
      {
        "name": "Workspace_Initial",
        "params": {
          "policyID": "CCDD48360B67D6DA"
        },
        "path": "settings/workspaces/CCDD48360B67D6DA"
      }
    ]
  }
}

We can get the deepest route which have the {name, params} by new method similar exist method getTopmostNestedRHPRoute. But I am not sure why we should do this, I think I need to dive in the code to get more about how this part works.

[ahmedGaber93]

After diving in the code, I found why templateStateTopmostFullScreen not have params attribute, this because when we get diffActions in getActionsFromPartialDiff unlike the others, we use the state object not only the nested route object{name, params}, and this to used in getActionFromState to determinined which navigation type (reset/navigate) will we use.

I think we need to get the route object{name, params} only to compare between states, and if it has diff we need to pass diff = last templateStateTopmostFullScreen not its nested route object, see code changes for more details.

new file/method: getTopmostFullScreenRoute

import NAVIGATORS from '@src/NAVIGATORS';
import type {FullScreenName, NavigationPartialRoute, RootStackParamList, State} from './types';

// Get the name of topmost fullscreen route in the navigation stack.
function getTopmostFullScreenRoute(state: State<RootStackParamList>): NavigationPartialRoute<FullScreenName> | undefined {
    if (!state) {
        return;
    }

    const topmostFullScreenRoute = state.routes.filter((route) => route.name === NAVIGATORS.FULL_SCREEN_NAVIGATOR).at(-1);

    if (!topmostFullScreenRoute) {
        return;
    }

    if (!!topmostFullScreenRoute.params && 'screen' in topmostFullScreenRoute.params) {
        return {name: topmostFullScreenRoute.params.screen as FullScreenName, params: topmostFullScreenRoute.params.params};
    }

    if (!topmostFullScreenRoute.state) {
        return;
    }

    // There will be at least one route in the central pane navigator.
    const {name, params} = topmostFullScreenRoute.state.routes.at(-1) as NavigationPartialRoute<FullScreenName>;

    return {name, params};
}

export default getTopmostFullScreenRoute;

isFullScreenNavigatorMandatory

if (metainfo.isFullScreenNavigatorMandatory) {
    const stateTopmostFullScreen = getTopmostFullScreenRoute(state)
    const templateStateTopmostFullScreen = getTopmostFullScreenRoute(templateState)

    // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
    if ((!stateTopmostFullScreen && templateStateTopmostFullScreen) ||
        (stateTopmostFullScreen &&
            templateStateTopmostFullScreen &&
            stateTopmostFullScreen.name !== templateStateTopmostFullScreen.name) &&
        !shallowCompare(stateTopmostFullScreen.params, templateStateTopmostFullScreen.params)
    ) {
        // we need to pass last templateState not its nested route only
        diff[NAVIGATORS.FULL_SCREEN_NAVIGATOR] = templateState.routes.filter((route) => route.name === NAVIGATORS.FULL_SCREEN_NAVIGATOR).at(-1) as NavigationPartialRoute;
    }
}

[shahinyan11]

@Ollyws Is there any feedback about my proposal

[Ollyws]

@shahinyan11 Your initial solution of using CONST.NAVIGATION.TYPE.UP seems like a workaround and your latest solution is similar to @ahmedGaber93's.

I think that's enough to move forward with @ahmedGaber93's proposal and we can refine the rest in the PR, as the other proposals both seem like derivatives of @ahmedGaber93's.

It would be nice if this didn't have to block the deploy though, as it's a bit fiddly and would be preferable not to be rushed. 🎀👀🎀 C+ reviewed

[melvin-bot[bot]]

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

[melvin-bot[bot]]

📣 @Ollyws 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

[melvin-bot[bot]]

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

[danieldoglas]

I don't feel like this is a deploy blocker too.

[ahmedGaber93]

@Ollyws PR https://github.com/Expensify/App/pull/39442 is ready for review.

[Ollyws]

This one was deployed to production two weeks ago can we get this paid and close? Thanks.

[melvin-bot[bot]]

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

[danieldoglas]

@kevinksullivan can you please help us out with the payments on this one? Thanks!

[melvin-bot[bot]]

@danieldoglas, @Ollyws, @kevinksullivan, @thienlnam, @ahmedGaber93 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[danieldoglas]

friendly bump again @kevinksullivan

[melvin-bot[bot]]

@danieldoglas, @Ollyws, @kevinksullivan, @thienlnam, @ahmedGaber93 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[melvin-bot[bot]]

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