Skip to content

Fix - Tax backwards compatibility - After disable Tax in OD display an it's not here page in ND #59565

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

Merged
merged 12 commits into from
May 8, 2025
Merged

Fix - Tax backwards compatibility - After disable Tax in OD display an it's not here page in ND #59565

Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
2c09110
navigate to more features on tax disabling
FitseTLT Apr 1, 2025
6525ee5
replace with goBack
FitseTLT Apr 1, 2025
e6c2646
Merge branch 'main' into fix-showing-not-found-when-tax-is-disabled-i…
FitseTLT Apr 2, 2025
72f3d0d
Merge branch 'main' into fix-showing-not-found-when-tax-is-disabled-i…
FitseTLT Apr 7, 2025
28e51d6
update comment
FitseTLT Apr 7, 2025
6b70649
Merge branch 'main' into fix-showing-not-found-when-tax-is-disabled-i…
FitseTLT Apr 11, 2025
4523c69
apply it to all features
FitseTLT Apr 11, 2025
276f183
Merge branch 'main' into fix-showing-not-found-when-tax-is-disabled-i…
FitseTLT Apr 23, 2025
77076c3
fix lint
FitseTLT Apr 23, 2025
6134427
minor fix
FitseTLT Apr 23, 2025
1b00abe
minor fix
FitseTLT Apr 23, 2025
e699b64
Merge branch 'main' into fix-showing-not-found-when-tax-is-disabled-i…
FitseTLT May 7, 2025
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
16 changes: 10 additions & 6 deletions src/pages/workspace/AccessOrNotFoundWrapper.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/* eslint-disable rulesdir/no-negated-variables */
import React, {useEffect, useState} from 'react';
import React, {useEffect} from 'react';
import type {OnyxCollection, OnyxEntry} from 'react-native-onyx';
import {useOnyx} from 'react-native-onyx';
import type {FullPageNotFoundViewProps} from '@components/BlockingViews/FullPageNotFoundView';
Expand Down Expand Up @@ -151,7 +151,6 @@ function AccessOrNotFoundWrapper({

const isFeatureEnabled = featureName ? isPolicyFeatureEnabledUtil(policy, featureName) : true;

const [isPolicyFeatureEnabled, setIsPolicyFeatureEnabled] = useState(isFeatureEnabled);
const {isOffline} = useNetwork();

const isPageAccessible = accessVariants.reduce((acc, variant) => {
Expand All @@ -160,15 +159,20 @@ function AccessOrNotFoundWrapper({
}, true);

const isPolicyNotAccessible = !isPolicyAccessible(policy);
const shouldShowNotFoundPage = (!isMoneyRequest && !isFromGlobalCreate && isPolicyNotAccessible) || !isPageAccessible || !isPolicyFeatureEnabled || shouldBeBlocked;
const shouldShowNotFoundPage = (!isMoneyRequest && !isFromGlobalCreate && isPolicyNotAccessible) || !isPageAccessible || shouldBeBlocked;
// We only update the feature state if it isn't pending.
// This is because the feature state changes several times during the creation of a workspace, while we are waiting for a response from the backend.
// Without this, we can have unexpectedly have 'Not Found' be shown.
// Without this, we can be unexpectedly navigated to the More Features page.
useEffect(() => {
if (pendingField && !isOffline && !isFeatureEnabled) {
if (isFeatureEnabled || (pendingField && !isOffline && !isFeatureEnabled)) {
return;
}
setIsPolicyFeatureEnabled(isFeatureEnabled);

// When a workspace feature linked to the current page is disabled we will navigate to the More Features page.
Navigation.isNavigationReady().then(() => Navigation.goBack(ROUTES.WORKSPACE_MORE_FEATURES.getRoute(policyID)));
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the purpose of using isNavigationReady here? Is there a scenario when the navigation isn't ready yet?

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 the scenario which caused the bug above on deep linking.

Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't look good, as the previous page flashes momentarily until navigation is ready

Screen.Recording.2025-04-29.at.21.34.16.mov
Screenshot 2025-04-29 at 21 38 24 Screenshot 2025-04-29 at 21 38 29

Could we show nothing (just the background) while navigation is not ready?

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 are overcomplicating it @eVoloshchak because what we are trying to achieve in this pr is navigating away from pages linked to a non-existing feature so the flash is totally expected and this is not also a common use case by the way a user deeplinking to a disabled feature.

Copy link
Contributor

@eVoloshchak eVoloshchak May 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It still doesn't look right, previously it worked without any flashing (not found page was shown right away). Could we implement it so that the not found page would show initially until the navigation is ready?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eVoloshchak previously we used to show not found page now what we are implementing is navigating away to more features and the above behavior is acceptable. Moreover, showing not found page until the navigation is ready doesn't help at all because we can see the previous page for an extremely small amount of duration, I think we are overthinking here.

@neil-marcellini WDYT about this. In my opinion, direct linking to a disabled feature is by itself very unlikely and if it happens what we are implementing in this pr is to navigate away to the more features page so the flashing is expected behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I think that's acceptable. I don't even see it as much of a flash myself. Maybe there's a way it could be perfected, but let's not bother with it for such a rare edge case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!
In that case, looks good to me, proceeding
@FitseTLT, could you resolve the conflicts please?

// We don't need to run the effect on policyID change as we only use it to get the route to navigate to.
// eslint-disable-next-line react-compiler/react-compiler
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [pendingField, isOffline, isFeatureEnabled]);

useEffect(() => {
Expand Down
Loading