Auto-select the Expensify Card tab when first creating a workspace

[kevinksullivan]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Join the freePlan beta
2. Create a New Workspace via the + icon in NewDot
3. Name the Workspace and arrive at the workspace menu with no tab auto-selected

Expected Result:

The Expensify Card tab of the Workspace should be Auto-selected when a user has just created one for the first time.

Actual Result:

No tab is selected when a user first sees the Workspace, and they arrive at the menu.

Platform:

Where is this issue occurring?

• Web
• iOS
• Android
• Desktop App
• Mobile Web

Notes/Photos/Videos:
Notice no tab is selected when first creating the workspace.

Expensify/Expensify Issue URL: https://github.com/Expensify/Expensify/issues/171670#

View all open jobs on Upwork

[MelvinBot]

Triggered auto assignment to @sonialiap (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[aman-atg]

Proposal

• This can be easily fixed by using constant strings inside menuItems
  

  App/src/pages/workspace/WorkspaceSidebar.js

  Line 54 in b08f80f

  isActive: Navigation.isActive(ROUTES.getWorkspaceCardRoute(policy.id)),

  
  We can change this line with isActive: Navigation.isWorkspaceActive(CONST.WORKSPACE.CARD)

• And a new isWorkspaceActive function can be use inside Navigation.js

function isWorkspaceActive(menuItemName) {
    const name = navigationRef.current.getCurrentRoute().name;
    return name === menuItemName;
}

• Constants

 WORKSPACE: {
       CARD: 'WorkspaceCard',
       PEOPLE: 'WorkspacePeople',
   },

• Suggestions are welcome as to where should these constants go and Proper naming for better readability and usability.

Maybe naming can be better

[rushatgabhane]

And a new isWorkspaceActive function can be use inside Navigation.js

@aman-atg That seems like a hack. Because Navigation.isActive() not working is the root cause.

[aman-atg]

@rushatgabhane IMO selecting sidebar items after parsing and comparing the routes is just an overkill for a simple task.

[rushatgabhane]

I mean, with your proposal we'll have to create a new constant and function every time we want to check if a route is active.
Does that make sense?

Anyway, check this out, getCurrentRoute().path is undefined the first time we visit the url.
(But gets defined on refresh/ clicking Expensify Card)

App/src/libs/Navigation/Navigation.js

Line 133 in b08f80f

const path = navigationRef.current && navigationRef.current.getCurrentRoute().path

[aman-atg]

new constant and function every time we want to check if a route is active

• I raised that concern by saying

Maybe naming can be better

• it could be isRouteName or something

Anyway, check this out, getCurrentRoute().path is undefined the first time we visit the url.
(But gets defined on refresh/ clicking Expensify Card)

And yeah I know that. What are ya trying to say here?
Also, just a point to note isActive isn't being used anywhere else

[mananjadhav]

While I feel this issue is an issue with react-navigation at the first glance.

** Proposal **

We can modify the isActive function that'll look something like this

function isActive(routePath) {
	let path = '';

	if (navigationRef.current && navigationRef.current.getCurrentRoute().path) {
		path = navigationRef.current.getCurrentRoute().path.substring(1)
	} else {
		let pathFromState = parsePathFromPathString(getPathFromState(navigationRef.current.getState()));
		if(pathFromState) {
			path = pathFromState;
		} 
	}

	return path === routePath;
}

function parsePathFromPathString(pathString) {

	// Parse path value from the following path string returned state
	// /WorkspaceSettings?initial=true&screen=WorkspacePeople&params=%5Bobject%20Object%5D&path=%2Fworkspace%2F51594D80CA931BBB%2Fpeopl
}

[rushatgabhane]

@mananjadhav We maintain currentURL as an ONYX key.

isActive() function can be a one-liner. What do you think?

[mananjadhav]

That’s the first thing that I checked and it works too.

I thought we would be using isActive later for internal screen navigation too. It it is going to be route based then Onyx works.

[mananjadhav]

Approach with Onyx:

Navigation.js

function isActive(routePath) {
  // We remove First forward slash from the URL before matching

  let currentUrl = getCurrentURL();
  const path =
    navigationRef.current && navigationRef.current.getCurrentRoute().path
      ? navigationRef.current.getCurrentRoute().path.substring(1)
      : currentUrl
      ? currentUrl.substring(1)
      : '';
  return path === routePath;
}

actions/App.js

let currentUrl;
Onyx.connect({
  key: ONYXKEYS.CURRENT_URL,
  callback: (val) => (currentUrl = val),
  initWithStoredValues: false,
});

function getCurrentURL() {
  return currentUrl;
}

While I tested and it worked fine. I am just concerned about one thing. Onyx uses a persistence layer with localStorage on the web. This is generally shared by the domain/application. So any updates on one open tab shouldn't affect the other.

[MelvinBot]

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

[iwiznia]

. Because Navigation.isActive() not working is the root cause.

I agree with this, given that when we create the workspace, we do:

Navigation.navigate(ROUTES.getWorkspaceCardRoute(response.policyID));

And then in the workspace card route, we do:

Navigation.isActive(ROUTES.getWorkspaceCardRoute(policy.id)),

So, that should return true... what I am not sure about is why this is happening... like why after changing the to the correct route/page the navigationRef is not properly updated?

[mananjadhav]

That's right and I did check that when it gets redirected first, the path is undefined in getCurrentRoute.

I checked until the linkTo where we dispatch the action. The payload matches exactly when we redirect from Create Workspace and also when I reload the page with the WorkspaceCardRoute path.

I gave some workaround solutions, but I feel this issue might require a little more digging.

[aman-atg]

@iwiznia

So, that should return true... what I am not sure about is why this is happening... like why after changing the to the correct route/page the navigationRef is not properly updated?

It might be happening because we're passing an empty object here

App/src/libs/Navigation/AppNavigator/WorkspaceSettingsDrawerNavigator.js

Line 17 in 69d740a

initialParams: {},

[aman-atg]

So, still the solution will be same, i.e. , passing a constant string so that it can be used later.

One example is here,

App/src/libs/Navigation/AppNavigator/AuthScreens.js

Line 391 in cd663c7

initialParams={{stepToOpen: CONST.BANK_ACCOUNT.STEP.BANK_ACCOUNT}}

[iwiznia]

@aman-atg passing a param sounds like a hack, this seems like something relating to our navigation is not totally correct. After you do Navigation.navigate(ROUTES.getWorkspaceCardRoute(response.policyID)); and right before the route is rendered, any call to Navigation.isActive(ROUTES.getWorkspaceCardRoute(policy.id)) should succeed.

So as @mananjadhav said, we need a bit more digging to see what exactly is going on.

[parasharrajat]

This seems an issue with React navigation. The path is not set on the first navigation.

[mananjadhav]

At the first attempt, I feel this line is what must be causing.

https://github.com/react-navigation/react-navigation/blob/6d518a46b89496f4a3bfd2da24245fe344f97290/packages/routers/src/CommonActions.tsx#L68

[iwiznia]

Can you expand on that @mananjadhav?

[mananjadhav]

I am not 100% sure of it. I only traced visually without executing. In the navigate action for this line, it only returns the name and the param, not the path. In the other case, it returns the complete payload.

Hence when I compared the payload we dispatch it is exactly the same. Hence I didn't comment my findings earlier in the thread.

[mananjadhav]

@iwiznia @parasharrajat While I couldn't find the root cause yet, I did find a way to get the correct path which is not a hack.

Based on the discussion in this thread #react-navigation/react-navigation#9312

import { useLinkBuilder } from '@react-navigation/native';

function isActive(routePath) {
	const buildLink = useLinkBuilder();
    // We remove First forward slash from the URL before matching
    const path = navigationRef.current && navigationRef.current.getCurrentRoute().name
        ? buildLink(navigationRef.current.getCurrentRoute().name, navigationRef.current.getCurrentRoute().params).substring(1)
        : '';
    return path === routePath;
}

The getPathFromState seems to be a problem. When I logged the path returned in NavigationRoot and in the Navigation.isActive it gives me different results.

NavigationRoot.parseAndStoreRoute

const path = getPathFromState(state, linkingConfig.config);
// Outputs:
// /workspace/51594D80CA931BBB/card

and Navigation.isActive

const path = getPathFromState(navigationRef.current.getState(), linkingConfig.config); // or even useNavigationState
// Outputs: 
// /workspace?initial=true&screen=WorkspaceCard&params=%5Bobject%20Object%5D&path=%2Fworkspace%2F51594D80CA931BBB%2Fcard

We can safely use useLinkBuilder and close this.

[parasharrajat]

I would say that missing path is not a issue as this is optional property. So we can't blame React navigation for that.
@mananjadhav Your solution seems a good to me. Building the path based out of active route is fine as we are not concerned with the node in navigation history. But you should rename this method to isActiveRoute to make this distinction. Also comments specifying the behavior would be good.

But useLinkBuilder is a hook. you have unfold it and then use it which is no issue. Internally it uses the same getPathFromState.

[mananjadhav]

I agree. path is an optional property. Yeah, I'll rename the method along with the comments. Yeah, I am aware internally it uses getPathFromState. But it'll be better if we use the hook rather than parsing the path. If you see the previous comment and the link, getPathFromState is giving different results. useLinkBuilder would be a safe choice

[mananjadhav]

@iwiznia Any feedback on the last approach ?

[iwiznia]

So does that solve the problem? Seems like no, since you say it's generating different results?

When I logged the path returned in NavigationRoot and in the Navigation.isActive it gives me different results.

[mananjadhav]

Using getPathFromState directly gives me an issue. Using useLinkBuilder from react-navigation resolves it. There are few additional lines on fetching the root navigation state in useLinkBuilder.

[iwiznia]

Oh I see... I am not sure I 100% understand the fix, but seems you do and that it works, so I'll check it out and test it when you submit it.
@kevinksullivan can you hire @mananjadhav in upworks please?

[mananjadhav]

Sure. It's after a lot of digging this one works properly rather than going through the getCurrentRoute approach.

[mananjadhav]

While @kevinksullivan posts a job and gives a shoutout, I've raised the PR to you @iwiznia. I've also attached relevant screencasts to show the outcome.

[kevinksullivan]

here you are @mananjadhav

https://www.upwork.com/jobs/~0193613b388ad040c2

[kevinksullivan]

Hired!

[iwiznia]

@mananjadhav please do not add Fixes before the issue number because that makes GH auto close it when merging, but we don't want this closed on merge, so just do $ #issueNumber

[mananjadhav]

Noted.

[MelvinBot]

@iwiznia Eep! 4 days overdue now. Issues have feelings too...

[iwiznia]

PR has been merged Melvin, leave me alone.

[kevinksullivan]

Paid @mananjadhav . Thanks!