Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add getPolicyIDFromNavigationState method #38490

Merged
merged 3 commits into from
Apr 1, 2024
Merged

Add getPolicyIDFromNavigationState method #38490

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
544ee16
Add getPolicyIDFromNavigationState method
WojtekBoman Mar 18, 2024
b71d2ad
Merge branch 'main' into fix/delayed-workspaces-transition
WojtekBoman Mar 22, 2024
eb11d74
Move getPolicyIDFromNavigationState to PolicyUtils
WojtekBoman Mar 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion src/libs/Navigation/Navigation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,14 @@ import type {Report} from '@src/types/onyx';
import type {EmptyObject} from '@src/types/utils/EmptyObject';
import originalDismissModal from './dismissModal';
import originalDismissModalWithReport from './dismissModalWithReport';
import getPolicyIDFromState from './getPolicyIDFromState';
import originalGetTopmostReportActionId from './getTopmostReportActionID';
import originalGetTopmostReportId from './getTopmostReportId';
import linkingConfig from './linkingConfig';
import linkTo from './linkTo';
import navigationRef from './navigationRef';
import switchPolicyID from './switchPolicyID';
import type {NavigationStateRoute, State, StateOrRoute, SwitchPolicyIDParams} from './types';
import type {NavigationStateRoute, RootStackParamList, State, StateOrRoute, SwitchPolicyIDParams} from './types';

let resolveNavigationIsReadyPromise: () => void;
const navigationIsReadyPromise = new Promise<void>((resolve) => {
Expand Down Expand Up @@ -359,6 +360,13 @@ function navigateWithSwitchPolicyID(params: SwitchPolicyIDParams) {
return switchPolicyID(navigationRef.current, params);
}

/**
* Returns the currently selected policy ID stored in the navigation state.
*/
function getPolicyIDFromNavigationState() {
return getPolicyIDFromState(navigationRef.getRootState() as State<RootStackParamList>);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the correct place for this function. This lib should only export helpers for navigation. This is a util function related to policy.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point :) I've moved this function to PolicyUtils


export default {
setShouldPopAllStateOnUP,
navigate,
Expand All @@ -379,6 +387,7 @@ export default {
closeFullScreen,
navigateWithSwitchPolicyID,
resetToHome,
getPolicyIDFromNavigationState,
};

export {navigationRef};
4 changes: 2 additions & 2 deletions src/pages/home/sidebar/SidebarScreen/BaseSidebarScreen.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import React, {useEffect} from 'react';
import {View} from 'react-native';
import ScreenWrapper from '@components/ScreenWrapper';
import useActiveWorkspace from '@hooks/useActiveWorkspace';
import useThemeStyles from '@hooks/useThemeStyles';
import * as Browser from '@libs/Browser';
import TopBar from '@libs/Navigation/AppNavigator/createCustomBottomTabNavigator/TopBar';
import Navigation from '@libs/Navigation/Navigation';
import Performance from '@libs/Performance';
import SidebarLinksData from '@pages/home/sidebar/SidebarLinksData';
import Timing from '@userActions/Timing';
Expand All @@ -21,7 +21,7 @@ const startTimer = () => {

function BaseSidebarScreen(props) {
const styles = useThemeStyles();
const {activeWorkspaceID} = useActiveWorkspace();
const activeWorkspaceID = Navigation.getPolicyIDFromNavigationState();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need the useActiveWorkspace context in the ideal nav v2?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need, because getting this value from the context should be faster than invoking this new method, because it has complex logic. In this place in code we need to get this value after update immediately but in other cases context should work fine. cc: @adamgrzybowski

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After some thought, I think we should check the following:

  • Is this method really slower? This is something that should be easy to check with console.time. Make sure to do this when the navigation state is big.
  • If we use this hook somewhere to trigger rerender.

If the answer is no and no then I would consider removing the context. It made sense to have it before ideal-nav-v2. Not sure about the current situation.

Keeping only one method to access policyID of the switcher may prevent confusion in the future.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@WojtekBoman any update on the comment above? That sounds like a good next step to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I've checked it in more detail, and the context works faster than this method. Here is necessary because we need to use a new policyID immediately after update, but in other cases it's better to use the context.

   const handleStateChange = (state: NavigationState | undefined) => {
        if (!state) {
            return;
        }
        const activeWorkspaceID = getPolicyIDFromState(state as NavigationState<RootStackParamList>);
        // Performance optimization to avoid context consumers to delay first render
        setTimeout(() => {
            currentReportIDValue?.updateCurrentReportID(state);
            setActiveWorkspaceID(activeWorkspaceID);
        }, 0);
        parseAndLogRoute(state);
    };

This is a method in the NavigationRoot.tsx file that updates the value stored in the context. In this particular case we need to read it faster and we can't wait for the update of this value, so we read it from the navigation state, but in other cases where we don't need to display this value it's fine.

useEffect(() => {
Performance.markStart(CONST.TIMING.SIDEBAR_LOADED);
Timing.start(CONST.TIMING.SIDEBAR_LOADED, true);
Expand Down
Loading