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

Hmm it is not here error appears briefly when enabling workflows #40219

Closed
wants to merge 13 commits into from
Closed

Hmm it is not here error appears briefly when enabling workflows #40219

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
f9845ed
Fix bug with Hmm it is not here error appears briefly when enabling w…
ZhenjaHorbach Apr 14, 2024
2344567
Refactor code
ZhenjaHorbach Apr 14, 2024
f8c53af
Fix comments
ZhenjaHorbach Apr 15, 2024
6f5b56d
Remove console.log
ZhenjaHorbach Apr 15, 2024
9e46077
Merge branch 'main' into error-appears-briefly-when-enabling-workflows
ZhenjaHorbach Apr 15, 2024
c7e87a6
Refactor changes
ZhenjaHorbach Apr 15, 2024
fc54584
Fix eslint issue
ZhenjaHorbach Apr 15, 2024
9568c9f
Fix rare bug with display not found
ZhenjaHorbach Apr 15, 2024
7484834
Rename const
ZhenjaHorbach Apr 15, 2024
268b870
Fix bug with not found after reload
ZhenjaHorbach Apr 16, 2024
92a32a7
Remove unnecessary space and check GPG key
ZhenjaHorbach Apr 16, 2024
601f0c7
Refactor changes and fix bug with not found after reload page and upd…
ZhenjaHorbach Apr 20, 2024
2785b4e
Merge branch 'main' into error-appears-briefly-when-enabling-workflows
ZhenjaHorbach Apr 20, 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
4 changes: 3 additions & 1 deletion src/libs/actions/Policy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3919,7 +3919,9 @@ function navigateWhenEnableFeature(policyID: string, featureRoute: Route) {
new Promise<void>((resolve) => {
resolve();
}).then(() => {
Navigation.navigate(featureRoute);
setTimeout(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have any other solution for this? We avoid using setTimeout

Copy link
Contributor Author

@ZhenjaHorbach ZhenjaHorbach Apr 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is the problem I described here

#40219 (comment)

And this problem is mentioned here

App/src/libs/actions/Policy.ts

Lines 3915 to 3918 in 268b870

/**
* The app needs to set a navigation action to the microtask queue, it guarantees to execute Onyx.update first, then the navigation action.
* More details - https://github.com/Expensify/App/issues/37785#issuecomment-1989056726.
*/

It's still reproduce (even on main branch )
When we navigate
We have cases when isFeatureEnabled equals false for a couple of milliseconds

So why don't we make the navigation happen after the switch animation ends?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's take a look at my video, I posted. I waited for all the request were called, so why are you saying that

Our request to enable this feature will not be completed because it was in the queue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry
My fault, I didn't notice that you waited for all the request were called
In this case, I fixed the cases you described )

Copy link
Contributor

@hungvu193 hungvu193 Apr 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think your explanation here is correct. Let's discuss our situation here, all APIs were called and we still saw the NotFound screen. That shouldn't happen and I think we can avoid using timeout to fix it.

Copy link
Contributor Author

@ZhenjaHorbach ZhenjaHorbach Apr 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I want to fix a specific case with these changes

When, after enabling a feature, we navigate immediately
That Onyx.update sometimes does not change the value immediately
As result we have NotFound for a couple of milliseconds

I think problem related with Promise which we use

App/src/libs/actions/Policy.ts

Lines 3915 to 3921 in 268b870

/**
* The app needs to set a navigation action to the microtask queue, it guarantees to execute Onyx.update first, then the navigation action.
* More details - https://github.com/Expensify/App/issues/37785#issuecomment-1989056726.
*/
new Promise<void>((resolve) => {
resolve();
}).then(() => {

But so as not to cause regression
I was thinking of using setTimeout inside Promise
To be sure that everything is called in the correct order

2024-04-17.13.39.19.mov

(Video is from main branch )

but okay )
I'll try to find an alternative solution )
Thank you for review )

Copy link
Contributor

Choose a reason for hiding this comment

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

Any update here?

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'll look into this issue thoroughly over the weekend
If you do not mind

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah yeah, np. Please keep me posted

Navigation.navigate(featureRoute);
}, 300);
});
}

Expand Down
24 changes: 22 additions & 2 deletions src/pages/workspace/FeatureEnabledAccessOrNotFoundWrapper.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
/* eslint-disable rulesdir/no-negated-variables */
import React, {useEffect} from 'react';
import {useIsFocused} from '@react-navigation/native';
import React, {useEffect, useState} from 'react';
import type {OnyxEntry} from 'react-native-onyx';
import {withOnyx} from 'react-native-onyx';
import FullPageNotFoundView from '@components/BlockingViews/FullPageNotFoundView';
import FullscreenLoadingIndicator from '@components/FullscreenLoadingIndicator';
import useNetwork from '@hooks/useNetwork';
import usePrevious from '@hooks/usePrevious';
import Navigation from '@libs/Navigation/Navigation';
import * as PolicyUtils from '@libs/PolicyUtils';
import * as Policy from '@userActions/Policy';
Expand Down Expand Up @@ -34,8 +37,16 @@ type FeatureEnabledAccessOrNotFoundComponentProps = FeatureEnabledAccessOrNotFou

function FeatureEnabledAccessOrNotFoundComponent(props: FeatureEnabledAccessOrNotFoundComponentProps) {
const isPolicyIDInRoute = !!props.policyID?.length;
const isFeatureEnabled = PolicyUtils.isPolicyFeatureEnabled(props.policy, props.featureName);
const [isPolicyFeatureEnabled, setIsPolicyFeatureEnabled] = useState(isFeatureEnabled);
const shouldShowFullScreenLoadingIndicator = props.isLoadingReportData !== false && (!Object.entries(props.policy ?? {}).length || !props.policy?.id);
const shouldShowNotFoundPage = isEmptyObject(props.policy) || !props.policy?.id || !PolicyUtils.isPolicyFeatureEnabled(props.policy, props.featureName);
const shouldShowNotFoundPage = isEmptyObject(props.policy) || !props.policy?.id || !isPolicyFeatureEnabled;
const {isOffline} = useNetwork();

const isFocused = useIsFocused();
const prevIsFocused = usePrevious(isFocused);
const pendingField = props.policy?.pendingFields?.[props.featureName];
const prevPendingField = usePrevious(pendingField);

useEffect(() => {
if (!isPolicyIDInRoute || !isEmptyObject(props.policy)) {
Expand All @@ -47,6 +58,15 @@ function FeatureEnabledAccessOrNotFoundComponent(props: FeatureEnabledAccessOrNo
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [isPolicyIDInRoute, props.policyID]);

useEffect(() => {
setIsPolicyFeatureEnabled((isCurrentPolicyFeatureEnabled) => {
if (prevPendingField !== pendingField || isOffline || !pendingField || !prevIsFocused) {
return isFeatureEnabled;
}
return isCurrentPolicyFeatureEnabled;
});
}, [pendingField, isFeatureEnabled, isOffline, prevPendingField, prevIsFocused]);

if (shouldShowFullScreenLoadingIndicator) {
return <FullscreenLoadingIndicator />;
}
Expand Down
51 changes: 45 additions & 6 deletions src/pages/workspace/WorkspaceInitialPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import OfflineWithFeedback from '@components/OfflineWithFeedback';
import ScreenWrapper from '@components/ScreenWrapper';
import ScrollView from '@components/ScrollView';
import useLocalize from '@hooks/useLocalize';
import useNetwork from '@hooks/useNetwork';
import usePermissions from '@hooks/usePermissions';
import usePrevious from '@hooks/usePrevious';
import useSingleExecution from '@hooks/useSingleExecution';
Expand All @@ -33,6 +34,7 @@ import ONYXKEYS from '@src/ONYXKEYS';
import ROUTES from '@src/ROUTES';
import SCREENS from '@src/SCREENS';
import type * as OnyxTypes from '@src/types/onyx';
import type {PolicyFeatureName} from '@src/types/onyx/Policy';
import {isEmptyObject} from '@src/types/utils/EmptyObject';
import type IconAsset from '@src/types/utils/IconAsset';
import type {WithPolicyAndFullscreenLoadingProps} from './withPolicyAndFullscreenLoading';
Expand Down Expand Up @@ -71,6 +73,20 @@ function WorkspaceInitialPage({policyDraft, policy: policyProp, policyMembers, r
const activeRoute = useNavigationState(getTopmostWorkspacesCentralPaneName);
const {translate} = useLocalize();
const {canUseAccountingIntegrations} = usePermissions();
const {isOffline} = useNetwork();

const prevPendingFields = usePrevious(policy?.pendingFields);
const policyFeatureStates = useMemo(
() => ({
[CONST.POLICY.MORE_FEATURES.ARE_DISTANCE_RATES_ENABLED]: policy?.areDistanceRatesEnabled,
[CONST.POLICY.MORE_FEATURES.ARE_WORKFLOWS_ENABLED]: policy?.areWorkflowsEnabled,
[CONST.POLICY.MORE_FEATURES.ARE_CATEGORIES_ENABLED]: policy?.areCategoriesEnabled,
[CONST.POLICY.MORE_FEATURES.ARE_TAGS_ENABLED]: policy?.areTagsEnabled,
[CONST.POLICY.MORE_FEATURES.ARE_TAXES_ENABLED]: policy?.tax?.trackingEnabled,
[CONST.POLICY.MORE_FEATURES.ARE_CONNECTIONS_ENABLED]: policy?.areConnectionsEnabled,
}),
[policy],
) as Record<PolicyFeatureName, boolean>;

const policyID = policy?.id ?? '';
const policyName = policy?.name ?? '';
Expand Down Expand Up @@ -108,6 +124,8 @@ function WorkspaceInitialPage({policyDraft, policy: policyProp, policyMembers, r
const isPaidGroupPolicy = PolicyUtils.isPaidGroupPolicy(policy);
const isFreeGroupPolicy = PolicyUtils.isFreeGroupPolicy(policy);

const [featureStates, setFeatureStates] = useState(policyFeatureStates);

const protectedFreePolicyMenuItems: WorkspaceMenuItem[] = [
{
translationKey: 'workspace.common.card',
Expand Down Expand Up @@ -152,7 +170,28 @@ function WorkspaceInitialPage({policyDraft, policy: policyProp, policyMembers, r

const protectedCollectPolicyMenuItems: WorkspaceMenuItem[] = [];

if (policy?.areDistanceRatesEnabled) {
useEffect(() => {
setFeatureStates((currentFeatureStates) => {
const newFeatureStates = {} as Record<PolicyFeatureName, boolean>;
const keys = Object.keys(policy?.pendingFields ?? {}) as PolicyFeatureName[];
keys.forEach((key) => {
const isFeatureEnabled = PolicyUtils.isPolicyFeatureEnabled(policy, key);
if (prevPendingFields?.[key] !== policy?.pendingFields?.[key] || isOffline || !policy?.pendingFields?.[key]) {
newFeatureStates[key] = isFeatureEnabled;

return;
}

newFeatureStates[key] = currentFeatureStates[key];
});
return {
...policyFeatureStates,
...newFeatureStates,
};
});
}, [policy, isOffline, policyFeatureStates, prevPendingFields]);

if (featureStates?.[CONST.POLICY.MORE_FEATURES.ARE_DISTANCE_RATES_ENABLED]) {
protectedCollectPolicyMenuItems.push({
translationKey: 'workspace.common.distanceRates',
icon: Expensicons.Car,
Expand All @@ -161,7 +200,7 @@ function WorkspaceInitialPage({policyDraft, policy: policyProp, policyMembers, r
});
}

if (policy?.areWorkflowsEnabled) {
if (featureStates?.[CONST.POLICY.MORE_FEATURES.ARE_WORKFLOWS_ENABLED]) {
protectedCollectPolicyMenuItems.push({
translationKey: 'workspace.common.workflows',
icon: Expensicons.Workflows,
Expand All @@ -171,7 +210,7 @@ function WorkspaceInitialPage({policyDraft, policy: policyProp, policyMembers, r
});
}

if (policy?.areCategoriesEnabled) {
if (featureStates?.[CONST.POLICY.MORE_FEATURES.ARE_CATEGORIES_ENABLED]) {
protectedCollectPolicyMenuItems.push({
translationKey: 'workspace.common.categories',
icon: Expensicons.Folder,
Expand All @@ -181,7 +220,7 @@ function WorkspaceInitialPage({policyDraft, policy: policyProp, policyMembers, r
});
}

if (policy?.areTagsEnabled) {
if (featureStates?.[CONST.POLICY.MORE_FEATURES.ARE_TAGS_ENABLED]) {
protectedCollectPolicyMenuItems.push({
translationKey: 'workspace.common.tags',
icon: Expensicons.Tag,
Expand All @@ -190,7 +229,7 @@ function WorkspaceInitialPage({policyDraft, policy: policyProp, policyMembers, r
});
}

if (policy?.tax?.trackingEnabled) {
if (featureStates?.[CONST.POLICY.MORE_FEATURES.ARE_TAXES_ENABLED]) {
protectedCollectPolicyMenuItems.push({
translationKey: 'workspace.common.taxes',
icon: Expensicons.Tax,
Expand All @@ -200,7 +239,7 @@ function WorkspaceInitialPage({policyDraft, policy: policyProp, policyMembers, r
});
}

if (policy?.areConnectionsEnabled && canUseAccountingIntegrations) {
if (featureStates?.[CONST.POLICY.MORE_FEATURES.ARE_CONNECTIONS_ENABLED] && canUseAccountingIntegrations) {
protectedCollectPolicyMenuItems.push({
translationKey: 'workspace.common.accounting',
icon: Expensicons.Sync,
Expand Down
Loading