[HOLD for payment 2024-07-30] [$250] [Search v1] Settings - Delay in avatar highlight and blank Search page when opening from Inbox

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Version Number: 9.0.3-1 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 Issue reported by: Applause - Internal Team

Action Performed:

1. Go to staging.new.expensify.com/settings/preferences/priority-mode
2. Click Account settings
3. Go to Inbox
4. Go to Search

Expected Result:

In Step 2, avatar will be instantly highlighted when clicking on Account settings In Step 4, Search page will open instantly

Actual Result:

In Step 2, avatar is not instantly highlighted when clicking on Account settings In Step 4, Search page opens with blank page for a few seconds

Workaround:

Unknown

Platforms:

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

• [ ] Android: Native
• [x] 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/98a5f17d-e2a1-417e-8081-f120ef64bae0

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~011ff0642a6c2f2946
• Upwork Job ID: 1806537464995654813
• Last Price Increase: 2024-06-28
• Automatic offers:
• ikevin127 | Reviewer | 102989312

Issue Owner

Current Issue Owner: @lschurr

[melvin-bot[bot]]

Triggered auto assignment to @robertjchen (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]

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

[lanitochka17]

We think that this bug might be related to #vip-vsp

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[nkdengineer]

Proposal

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

In Step 2, avatar is not instantly highlighted when clicking on Account settings In Step 4, Search page opens with blank page for a few seconds

What is the root cause of that problem?

The check to show loading isn't correct.

https://github.com/Expensify/App/blob/1d607acab9c47f2e0da4e8c85de5d90e2d47c11b/src/components/Search.tsx#L95

isLoadingOnyxValue(searchResultsMeta) is only true at the first time this key is loaded in Onyx. So after the first time we open the search page, the blank will appear when the search API is called.

About the delay bug, I think it maybe related here https://expensify.slack.com/archives/C01GTK53T8Q/p1719497873778469

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

We should change to show the loading if the onyx value is loading or the search api is calling

const isLoadingItems = (!isOffline && (isLoadingOnyxValue(searchResultsMeta) || currentSearchResults?.search.isLoading)) || searchResults?.data === undefined;

https://github.com/Expensify/App/blob/1d607acab9c47f2e0da4e8c85de5d90e2d47c11b/src/components/Search.tsx#L95

What alternative solutions did you explore? (Optional)

NA

[mountiny]

cc @luacmartins

[luacmartins]

I wonder if we can simplify the loading condition to just !isOffline && searchResults?.data === undefined. I think we should have data defined in all other instances when we got a response from the API so that would either display the empty state or the data

[ikevin127]

@luacmartins' suggestion is a definite improvement on the Search page loading logic.

Regarding the bottom tam navigator tab switch UI delay, it still happens and I checked the slack discussion about the recent introduction of React StrictMode and this does not seem to be the issue as the behaviour is also present on latest staging.

Not sure which direction we should move forward with this issue:

1. Go with nkdengineer's proposal which would only improve the Search page loading logic.
2. Keep looking for proposals for one that would fix the issue entirely including the bottom tab navigator tab switch delays.

cc @mountiny @robertjchen

[Zakpak0]

Proposal

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

On the Expensify Website, the bottom tab navigator has a significant delay when switching tabs.

(a) The Account avatar is not highlighted immediately.

(b) The search tab renders content after an unwarranted delay.

What is the root cause of that problem?

(a) The navigation state is not being reported to child components promptly.

(b) Rendering logic needs to be improved for the search tab.

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

(a) Within the tab navigator component, the current route is being reported via the ActiveTabContext. We can utilize this to fix our problem with a few edits. This should be renamed to ActiveBottomTabContext as it only ever reports the state of the bottom tab. Additionally, the navigation state currently fed to this Context is a bit limited in scope. It should be expanded to report all changes to the bottom tabs vs just the routes that are referred to as CentralPlaneRoutes Now when we useContext in our child components we will get an immediate report and re-render the state. This will show the Account avatar highlighted immediately on tab change, rather than after a delay.

(b) Apply the solution provided in comment by @luacmartins

The solution can be reviewed here on my test branch

What alternative solutions did you explore? (Optional)

N/A

[melvin-bot[bot]]

📣 @Zakpak0! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

1. Make sure you've read and understood the contributing guidelines.
2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Format:

   Contributor details
   Your Expensify account email: <REPLACE EMAIL HERE>
   Upwork Profile Link: <REPLACE LINK HERE>

[Zakpak0]

Contributor details Your Expensify account email: zakharyoliver808@gmail.com Upwork Profile Link: (https://www.upwork.com/freelancers/~0144a75be415af6251)

[melvin-bot[bot]]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[melvin-bot[bot]]

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

[ikevin127]

@Zakpak0 Thanks for the proposal.

(a) Within the tab navigator component, the current route is being reported via the ActiveTabContext. We should useContext in our child components to get an immediate report and re-render the state. This will show the Account avatar highlighted immediately on tab change, rather than after a delay.

(b) Unmounting the inbox component on tab change should cancel pending callbacks. If we again use the ActiveTabContext to report the tab as changed, we can unblock the thread faster and remove the rendering delay.

In order for me to be able to review a proposal and be able to test whether it works in fixing our issue I need a more detailed explanation of your solution, there are 2 ways you can go about this:

1. Create a test branch on your Expensify/App fork where you apply the changes you detailed in your solution, then post the link to your fork branch here in a comment, this way I can test whether your solution fixes the issue.

   Note: Do not create a pull request, just a branch on your Expensify/App fork where you push the changes.

2. You can post a comment here detailing step by step, with code references if possible, where all the changes mentioned in your solution ([a.] & [b.]) should be applied in Expensify/App's codebase, this way I can apply the changes locally and verify whether your solution fixes the issue.

[Zakpak0]

@ikevin127 No problem, here you are! Is it ok if I do a little bit of both? https://github.com/Zakpak0/Expensify-App-Fork/tree/zakpak0/bugfix-issue-%2344587

(a) src/libs/Navigation/AppNavigator/createCustomBottomTabNavigator/BottomTabBar/index.website.tsx Here I imported the useActiveRoute hook and use it to check the name of the current route. Then we match that to the tab we want highlighted.

 /**
  * Use active route context in web because useNavigationState events aren't being fired immediately on tab change.
 */
    const activeRoute = useActiveRoute();

    const chatTabBrickRoad = getChatTabBrickRoad(activeWorkspaceID);

    const navigateToChats = useCallback(() => {
        if (currentTabName === SCREENS.HOME) {
            return;
        }
        const route = activeWorkspaceID ? (`/w/${activeWorkspaceID}/home` as Route) : ROUTES.HOME;
        Navigation.navigate(route);
    }, [activeWorkspaceID, currentTabName]);

    return (
        <View style={styles.bottomTabBarContainer}>
            <Tooltip text={translate('common.inbox')}>
                <PressableWithFeedback
                    onPress={navigateToChats}
                    role={CONST.ROLE.BUTTON}
                    accessibilityLabel={translate('common.inbox')}
                    wrapperStyle={styles.flex1}
                    style={styles.bottomTabBarItem}
                >
                    <View>
                        <Icon
                            src={Expensicons.Inbox}
                            fill={activeRoute?.name.includes('Report') ? theme.iconMenu : theme.icon}
                            width={variables.iconBottomBar}
                            height={variables.iconBottomBar}
                        />
                        {chatTabBrickRoad && (
                            <View style={styles.bottomTabStatusIndicator(chatTabBrickRoad === CONST.BRICK_ROAD_INDICATOR_STATUS.INFO ? theme.iconSuccessFill : theme.danger)} />
                        )}
                    </View>
                </PressableWithFeedback>
            </Tooltip>
            <Tooltip text={translate('common.search')}>
                <PressableWithFeedback
                    onPress={() => {
                        if (currentTabName === SCREENS.SEARCH.BOTTOM_TAB || currentTabName === SCREENS.SEARCH.CENTRAL_PANE) {
                            return;
                        }
                        interceptAnonymousUser(() => Navigation.navigate(ROUTES.SEARCH.getRoute(CONST.SEARCH.TAB.ALL)));
                    }}
                    role={CONST.ROLE.BUTTON}
                    accessibilityLabel={translate('common.search')}
                    wrapperStyle={styles.flex1}
                    style={styles.bottomTabBarItem}
                >
                    <View>
                        <Icon
                            src={Expensicons.MoneySearch}
                            fill={activeRoute?.name.includes('Search') ? theme.iconMenu : theme.icon}
                            width={variables.iconBottomBar}
                            height={variables.iconBottomBar}
                        />
                    </View>
                </PressableWithFeedback>
            </Tooltip>
            <BottomTabAvatar isSelected={activeRoute?.name.includes('Settings')} />
            <View style={[styles.flex1, styles.bottomTabBarItem]}>
                <BottomTabBarFloatingActionButton />
            </View>
        </View>
    );

(b) src/pages/home/sidebar/SidebarScreen/index.tsx Here I imported the useActiveRoute hook again and to check the name of the current route. On the surface it seems that when the route is either "Report" or undefined, we are in the Inbox tab. So I created a condition to return the component as null when those aren't the cases.

/**
 * In web, useActiveRoute reports tab change immediately.
 */
const route = useActiveRoute();

/**
 * Immediately unmount component on tab change to avoid blocking JS thread.
 */
if (route?.name !== 'Report' && !!route?.name) {
    return null;
}

[ikevin127]

@Zakpak0 Thanks for the update!

Currently the proposed solution for (a.) using useActiveRoute seems to work on web (wide layout devices) but if we switch to mWeb (mobile - narrow layouts) where the bottom tab navigation works differently, the solution would cause a regression if implemented for the BottomTabAvatar highlight because activeRoute?.name is undefined (see video below).

MacOS: Chrome

Regarding solution for (b.) -> I don't think those changes are necessary as the Search page part could be improved by the suggestion made in https://github.com/Expensify/App/issues/44587#issuecomment-2197152916.

Summary

1. The direction of using useActiveRoute seems good, but it has to work on mWeb as well. Checkout SCREENS.ts to potentially improve the solution by using navigation specific constants (includes('Settings') is not recommended) and take note of the currentTabName logic in order to better understand how that works.
2. For the second problem (b.) the suggestion made in #44587 (comment) should be enough, as the main focus should be the highlights delay.

[Zakpak0]

@ikevin127 thank you for the feed back. I apprecaite the call out there. I updated the branch with a bit of refactoring. src/libs/Navigation/AppNavigator/Navigators/BottomTabNavigator.tsx The previous navigation state selector was limited in the screens it was selecting. I followed the styling from the previous implementation and used the SCREENS.ts constants to define the routes.

const activeRoute = useNavigationState<RootStackParamList, NavigationPartialRoute<keyof BottomTabScreensParamList> | undefined>((state) => {
        if (!state) {
            return undefined;
        }
        let route: NavigationPartialRoute<keyof BottomTabScreensParamList> | undefined;
        for (let selector of [getTopmostBottomTabRoute, getTopmostCentralPaneRoute]) {
            const selectedRoute = selector(state);
            if (isBottomTabName(selectedRoute?.name)) {
                route = selectedRoute as NavigationPartialRoute<keyof BottomTabScreensParamList>;
            }
        }

        return route;
    });
}

src/libs/NavigationUtils.ts I also created helper functions to determine whether the selected tab falls within whichever category.

const SETTINGS_SCREENS = Object.values(flattenObject(SCREENS.SETTINGS));
const SEARCH_SCREENS = Object.values(flattenObject(SCREENS.SEARCH));
const HOME_SCREENS = [SCREENS.HOME, SCREENS.REPORT];
const BOTTOM_TAB_SCREEN_NAMES = new Set([...SETTINGS_SCREENS, ...SEARCH_SCREENS, ...HOME_SCREENS]);

const SETTINGS_TAB_SCREEN_NAMES = new Set(SETTINGS_SCREENS);

const SEARCH_TAB_SCREEN_NAMES = new Set(SEARCH_SCREENS);

const HOME_SCREEN_NAMES = new Set(HOME_SCREENS);

function isBottomTabName(screen: string | undefined): screen is BottomTabScreenName {
    if (!screen) {
        return false;
    }
    return BOTTOM_TAB_SCREEN_NAMES.has(screen as any);
}

function isSettingTabName(screen: string | undefined): screen is any {
    if (!screen) {
        return false;
    }
    return SETTINGS_TAB_SCREEN_NAMES.has(screen as any);
}

function isSearchTabName(screen: string | undefined): screen is (typeof SEARCH_SCREENS)[0] {
    if (!screen) {
        return false;
    }
    return SEARCH_TAB_SCREEN_NAMES.has(screen as (typeof SEARCH_SCREENS)[0]);
}

function isHomeTabName(screen: string | undefined): screen is (typeof HOME_SCREENS)[0] {
    if (!screen) {
        return false;
    }
    return HOME_SCREEN_NAMES.has(screen as (typeof HOME_SCREENS)[0]);

src/libs/Navigation/types.ts Corresponding types were exported to match the style of the previous implimentation.

src/hooks/useActiveBottomTabRoute.ts src/libs/Navigation/AppNavigator/Navigators/ActiveBottomTabRouteContext.ts I updated the name of useActiveRoute to useActiveBottomTabRoute to make it more identifiable. It only ever relayed information about the bottom tabs and I didn't want to be misleading.

src/components/Search/index.tsx Lastly I implemented the code from @luacmartins suggestion and removed the code from my implementation.

[luacmartins]

Thanks @Zakpak0. @ikevin127 could you please test the solution above when you get a chance?

[ikevin127]

@Zakpak0 Thanks for the follow-up! I reviewed your changes and I confirm that your changes fix the highlight delay issue on both wide and narrow layout devices as can be observed below.

Before	After

Notes on the code

• addition of new flattenObject function which is not needed as we already have it in @src/languages/translations
• noticed some typing errors mainly in NavigationUtils.ts
• minor naming improvements required

I took the liberty of correcting all of these, below is the diff which you should apply to your current test branch:

DIFF

```diff diff --git a/src/libs/Navigation/AppNavigator/Navigators/BottomTabNavigator.tsx b/src/libs/Navigation/AppNavigator/Navigators/BottomTabNavigator.tsx index 24d8afca10..372c1ce478 100644 --- a/src/libs/Navigation/AppNavigator/Navigators/BottomTabNavigator.tsx +++ b/src/libs/Navigation/AppNavigator/Navigators/BottomTabNavigator.tsx @@ -26,7 +26,7 @@ function BottomTabNavigator() { return undefined; } let route: NavigationPartialRoute | undefined; - for (let selector of [getTopmostBottomTabRoute, getTopmostCentralPaneRoute]) { + for (const selector of [getTopmostBottomTabRoute, getTopmostCentralPaneRoute]) { const selectedRoute = selector(state); if (isBottomTabName(selectedRoute?.name)) { route = selectedRoute as NavigationPartialRoute; diff --git a/src/libs/Navigation/AppNavigator/createCustomBottomTabNavigator/BottomTabBar/index.website.tsx b/src/libs/Navigation/AppNavigator/createCustomBottomTabNavigator/BottomTabBar/index.website.tsx index 995be14834..b27c5bc9df 100644 --- a/src/libs/Navigation/AppNavigator/createCustomBottomTabNavigator/BottomTabBar/index.website.tsx +++ b/src/libs/Navigation/AppNavigator/createCustomBottomTabNavigator/BottomTabBar/index.website.tsx @@ -3,11 +3,12 @@ import React, {useCallback, useEffect} from 'react'; import {View} from 'react-native'; import type {OnyxEntry} from 'react-native-onyx'; import {withOnyx} from 'react-native-onyx'; +import type {TupleToUnion} from 'type-fest'; import Icon from '@components/Icon'; import * as Expensicons from '@components/Icon/Expensicons'; import {PressableWithFeedback} from '@components/Pressable'; import Tooltip from '@components/Tooltip'; -import useActiveRoute from '@hooks/useActiveBottomTabRoute'; +import useActiveBottomTabRoute from '@hooks/useActiveBottomTabRoute'; import useActiveWorkspace from '@hooks/useActiveWorkspace'; import useLocalize from '@hooks/useLocalize'; import useTheme from '@hooks/useTheme'; @@ -41,6 +42,7 @@ function BottomTabBar({isLoadingApp = false}: PurposeForUsingExpensifyModalProps const styles = useThemeStyles(); const {translate} = useLocalize(); const navigation = useNavigation(); + const HOME_SCREENS = [SCREENS.HOME, SCREENS.REPORT]; const {activeWorkspaceID: contextActiveWorkspaceID} = useActiveWorkspace(); const activeWorkspaceID = sessionStorage.getItem(CONST.SESSION_STORAGE_KEYS.ACTIVE_WORKSPACE_ID) ?? contextActiveWorkspaceID; @@ -70,10 +72,7 @@ function BottomTabBar({isLoadingApp = false}: PurposeForUsingExpensifyModalProps return topmostBottomTabRoute?.name ?? SCREENS.HOME; }); - /** - * Use active route context in web because useNavigationState events aren't being fired immediately on tab change. - */ - const activeRoute = useActiveRoute(); + const activeBottomTabRoute = useActiveBottomTabRoute(); const chatTabBrickRoad = getChatTabBrickRoad(activeWorkspaceID); const navigateToChats = useCallback(() => { @@ -97,7 +96,7 @@ function BottomTabBar({isLoadingApp = false}: PurposeForUsingExpensifyModalProps ) ? theme.iconMenu : theme.icon} width={variables.iconBottomBar} height={variables.iconBottomBar} /> @@ -110,7 +109,7 @@ function BottomTabBar({isLoadingApp = false}: PurposeForUsingExpensifyModalProps { - if (isSearchTabName(activeRoute?.name)) { + if (isSearchTabName(activeBottomTabRoute?.name)) { return; } interceptAnonymousUser(() => Navigation.navigate(ROUTES.SEARCH.getRoute(CONST.SEARCH.TAB.ALL))); @@ -123,14 +122,14 @@ function BottomTabBar({isLoadingApp = false}: PurposeForUsingExpensifyModalProps - + diff --git a/src/libs/NavigationUtils.ts b/src/libs/NavigationUtils.ts index 8a9e2f9bab..34e9df9546 100644 --- a/src/libs/NavigationUtils.ts +++ b/src/libs/NavigationUtils.ts @@ -1,8 +1,9 @@ import cloneDeep from 'lodash/cloneDeep'; +import type {TupleToUnion} from 'type-fest'; +import {flattenObject} from '@src/languages/translations'; import SCREENS from '@src/SCREENS'; -import {flattenObject} from './flattenObject'; import getTopmostBottomTabRoute from './Navigation/getTopmostBottomTabRoute'; -import type {BottomTabScreenName, CentralPaneName, RootStackParamList, State} from './Navigation/types'; +import type {CentralPaneName, RootStackParamList, State} from './Navigation/types'; const CENTRAL_PANE_SCREEN_NAMES = new Set([ SCREENS.SETTINGS.WORKSPACES, @@ -46,32 +47,32 @@ const SEARCH_TAB_SCREEN_NAMES = new Set(SEARCH_SCREENS); const HOME_SCREEN_NAMES = new Set(HOME_SCREENS); -function isBottomTabName(screen: string | undefined): screen is BottomTabScreenName { +function isBottomTabName(screen: TupleToUnion | undefined) { if (!screen) { return false; } - return BOTTOM_TAB_SCREEN_NAMES.has(screen as any); + return BOTTOM_TAB_SCREEN_NAMES.has(screen); } -function isSettingTabName(screen: string | undefined): screen is any { +function isSettingTabName(screen: TupleToUnion | undefined) { if (!screen) { return false; } - return SETTINGS_TAB_SCREEN_NAMES.has(screen as any); + return SETTINGS_TAB_SCREEN_NAMES.has(screen); } -function isSearchTabName(screen: string | undefined): screen is (typeof SEARCH_SCREENS)[0] { +function isSearchTabName(screen: TupleToUnion | undefined) { if (!screen) { return false; } - return SEARCH_TAB_SCREEN_NAMES.has(screen as (typeof SEARCH_SCREENS)[0]); + return SEARCH_TAB_SCREEN_NAMES.has(screen); } -function isHomeTabName(screen: string | undefined): screen is (typeof HOME_SCREENS)[0] { +function isHomeTabName(screen: TupleToUnion | undefined) { if (!screen) { return false; } - return HOME_SCREEN_NAMES.has(screen as (typeof HOME_SCREENS)[0]); + return HOME_SCREEN_NAMES.has(screen); } export {isCentralPaneName, isBottomTabName, isSearchTabName, isSettingTabName, isHomeTabName, removePolicyIDParamFromState}; diff --git a/src/pages/Search/SearchPageBottomTab.tsx b/src/pages/Search/SearchPageBottomTab.tsx index 58a8c54c0c..ad95824fa6 100644 --- a/src/pages/Search/SearchPageBottomTab.tsx +++ b/src/pages/Search/SearchPageBottomTab.tsx @@ -27,7 +27,7 @@ const defaultSearchProps = { function SearchPageBottomTab() { const {translate} = useLocalize(); const {isSmallScreenWidth} = useWindowDimensions(); - const activeRoute = useActiveBottomTabRoute(); + const activeBottomTabRoute = useActiveBottomTabRoute(); const styles = useThemeStyles(); const { @@ -36,11 +36,11 @@ function SearchPageBottomTab() { sortBy, sortOrder, } = useMemo(() => { - if (activeRoute?.name !== SCREENS.SEARCH.CENTRAL_PANE || !activeRoute.params) { + if (activeBottomTabRoute?.name !== SCREENS.SEARCH.CENTRAL_PANE || !activeBottomTabRoute.params) { return defaultSearchProps; } - return {...defaultSearchProps, ...activeRoute.params} as SearchPageProps['route']['params']; - }, [activeRoute]); + return {...defaultSearchProps, ...activeBottomTabRoute.params} as SearchPageProps['route']['params']; + }, [activeBottomTabRoute]); const query = rawQuery as SearchQuery; diff --git a/src/pages/settings/InitialSettingsPage.tsx b/src/pages/settings/InitialSettingsPage.tsx index d3b9e28fe6..ef781d7813 100755 --- a/src/pages/settings/InitialSettingsPage.tsx +++ b/src/pages/settings/InitialSettingsPage.tsx @@ -1,11 +1,11 @@ import {useRoute} from '@react-navigation/native'; import React, {useCallback, useContext, useEffect, useLayoutEffect, useMemo, useRef, useState} from 'react'; +// eslint-disable-next-line no-restricted-imports import type {GestureResponderEvent, ScrollView as RNScrollView, ScrollViewProps, StyleProp, ViewStyle} from 'react-native'; import {NativeModules, View} from 'react-native'; import type {OnyxCollection, OnyxEntry} from 'react-native-onyx'; import {withOnyx} from 'react-native-onyx'; import type {ValueOf} from 'type-fest'; -// eslint-disable-next-line no-restricted-imports import AvatarWithImagePicker from '@components/AvatarWithImagePicker'; import ConfirmModal from '@components/ConfirmModal'; import CurrentUserPersonalDetailsSkeletonView from '@components/CurrentUserPersonalDetailsSkeletonView'; @@ -105,7 +105,7 @@ function InitialSettingsPage({session, userWallet, bankAccountList, fundList, wa const waitForNavigate = useWaitForNavigation(); const popoverAnchor = useRef(null); const {translate, formatPhoneNumber} = useLocalize(); - const activeRoute = useActiveBottomTabRoute(); + const activeBottomTabRoute = useActiveBottomTabRoute(); const emojiCode = currentUserPersonalDetails?.status?.emojiCode ?? ''; const [shouldShowSignoutConfirmModal, setShouldShowSignoutConfirmModal] = useState(false); @@ -332,7 +332,11 @@ function InitialSettingsPage({session, userWallet, bankAccountList, fundList, wa hoverAndPressStyle={styles.hoveredComponentBG} shouldBlockSelection={!!item.link} onSecondaryInteraction={item.link ? (event) => openPopover(item.link, event) : undefined} - focused={!!activeRoute && !!item.routeName && !!(activeRoute.name.toLowerCase().replaceAll('_', '') === item.routeName.toLowerCase().replaceAll('/', ''))} + focused={ + !!activeBottomTabRoute && + !!item.routeName && + !!(activeBottomTabRoute.name.toLowerCase().replaceAll('_', '') === item.routeName.toLowerCase().replaceAll('/', '')) + } isPaneMenu iconRight={item.iconRight} shouldShowRightIcon={item.shouldShowRightIcon} @@ -352,7 +356,7 @@ function InitialSettingsPage({session, userWallet, bankAccountList, fundList, wa userWallet?.currentBalance, isExecuting, singleExecution, - activeRoute, + activeBottomTabRoute, waitForNavigate, ], ); ```

What's next ?

1. Zakpak0 Should should apply the DIFF changes to the test branch then update their proposal (by edit) to reflect the latest changes in both RCA and solution with all relevant details. Let us know when the proposal was updated.
2. Once (1.) is completed I will perform a final review and if all good -> will move on with assignment.
3. (Optional) @Expensify/design to chime in regarding the blank Search page UI and possibly propose some sort of loader, I was thinking skeleton could look good there, since even with @luacmartins's suggested change applied from #44587 (comment), sometimes it can still take a bit until the list is rendered -> which shows the blank page (see below).

MacOS: Chrome

https://github.com/Expensify/App/assets/56457735/459ce10a-df20-4a2b-ab8b-85ec521f701c

[Zakpak0]

Proposal

Updated

@ikevin127 thank you for the feedback again!

[ikevin127]

@Zakpak0's updated proposal looks good to me. The root cause was identified and the updated solution fixes the issue as per the expected result. Some before / after of the solution can be seen in https://github.com/Expensify/App/issues/44587#issuecomment-2207196137.

@Zakpak0 The raw proposal template suggest that it's not allowed to post diffs in proposals, that's why I advised to use the diff and apply it to your test branch, which you are allowed to showcase in a proposal.

I think the design part (if needed) and other details can be handled during PR.

🎀👀🎀 C+ reviewed

[melvin-bot[bot]]

Current assignees @robertjchen and @luacmartins are eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[Zakpak0]

Proposal

Updated

@ikevin127 thank you again for the callout!!!

[shawnborton]

Regarding the blank search page, curious from @luacmartins why we aren't using our skeleton loaders there? I could have sworn we used to be doing that, in fact we even went as far as updating the skeleton loader for mobile to make it match the shape of the mobile rows. Any idea where they went?!

[luacmartins]

It seems like we do show the loading skeleton briefly but then it disappears and the page goes blank before loading the content. I wonder if the following is happening:

1. No data locally
2. Loading skeleton is shown
3. API request is sent
4. API request returns the data
5. Data is merged locally and loading skeleton disappears
6. List is large so the device takes a while to render the content, in the meantime page appears blank

If that's the issue then we have a performance issue and should probably tackle that as a separate issue.

[melvin-bot[bot]]

📣 @ikevin127 🎉 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]]

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

[shawnborton]

Tackling that separately works for me.

[Zakpak0]

Hey, the PR for this can be ready for review today at EOD for me.

[Zakpak0]

Update

Pending receiving the job offer on Upwork.

[ikevin127]

@Zakpak0 Don't worry too much about the Upwork offer / contract, rest assured -> you will get paid.

Being your first contract, it is not automated yet, as the bot mentioned in https://github.com/Expensify/App/issues/44587#issuecomment-2209151897 - that will change with your 2nd assignment.

You should move forward with opening the PR, the sooner the better because the way things will work is:

1. You open PR, completing your PR author checklist.
2. I review, test, complete my PR reviewer checklist then Approve.
3. Once I Approve, an Expensify engineer will double-check and if all good, it will get merged.
4. Once merged, the QA team will do some testing and after that the PR will be deployed to Staging (can take 1-2 days).
5. Once deployed to staging, some more testing will be done, after which it will be deployed to Production.
6. Once deployed to Production, the GH automation should add a [HOLD for payment X] to the issue's title - which is always 7 days after the PR was deployed to production.

If 7 days pass without any regressions, that's when your payment will be issued.

Being your first job, the contract will have to be handled manually by the BZ team member once the 7-day regression period is over.

How is it going to be different in the future ? The next time you get assigned to an issue as Contributor, the automation will send you an Upwork contract which you will be able to accept - the job will be showing-up in your Upwork - Work in progress.

So you won't need to apply to the Upwork job like the first time.

[Zakpak0]

@ikevin127 Oh got it! Thanks. I'll send the PR in then. Thank you so much for clarifying.

[melvin-bot[bot]]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot[bot]]

The solution for this issue has been :rocket: deployed to production :rocket: in version 9.0.7-8 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

• https://github.com/Expensify/App/pull/44931

If no regressions arise, payment will be issued on 2024-07-24. :confetti_ball:

For reference, here are some details about the assignees on this issue:

• @ikevin127 requires payment automatic offer (Reviewer)
• @Zakpak0 requires payment (Needs manual offer from BZ)

[ikevin127]

@luacmartins @robertjchen Can you please add the Bug label to assign somebody for payments ?

Note: I don't think we need a BZ Checklist here since the issue wasn't caused by any recent PR, but instead it's more of a performance improvement task.

[melvin-bot[bot]]

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

[lschurr]

@Zakpak0 - can you link your Upwork profile here so that I can send along the offer?

[lschurr]

Payment summary:

• Contributor: @Zakpak0 $250 (paid in Upwork)
• Reviewer: @ikevin127 $250 (paid in Upwork)

[Zakpak0]

@Zakpak0 - can you link your Upwork profile here so that I can send along the offer?

@lschurr sure https://www.upwork.com/freelancers/~0144a75be415af6251

[ikevin127]

@lschurr 👋 Before closing here, there's one more payment due on 30th coming from a follow-up polishing PR https://github.com/Expensify/App/pull/45663 which I reviewed. More context on the payment due date here and confirmation that it should be handled in this issue from Carlos in https://github.com/Expensify/App/pull/45663#issuecomment-2249241153.

⚠️ Looks like automation failed -> reason for which the 2nd payment due date wasn't added to this issue automatically.

[lschurr]

@Zakpak0 - sent offer here in Upwork

[lschurr]

I'll change the date in the title for the other payment that's due on the 30th. That PR was created by @WojtekBoman and reviewed by @ikevin127 - so we just need payment for you, right Kevin?

[ikevin127]

@lschurr Yes, only for me as reviewer since the author is from SWM. Thank you!

[ikevin127]

cc @lschurr

[lschurr]

@ikevin127 - I added a bonus rather than sending a new offer on this one.

Additional $250 paid in Upwork for this PR.