[$250] Workspace -App redirects to workspace editor with error when returning from not here page

[izarutskaya]

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.15-4 Reproducible in staging?: Y Reproducible in production?: Y Found when executing PR : https://github.com/Expensify/App/pull/46479 Logs: https://stackoverflow.com/c/expensify/questions/4856 Issue reported by: Applause-Internal team

Action Performed:

1. Launch New Expensify app.
2. Paste https://github.com/Expensify/App/pull/46479 in any chat.
3. Tap on the link.
4. Swipe back to return to previous page (not app back button).
5. Might need to repeat Step 3 and 4 one more time to see the workspace editor.

Expected Result:

In Step 4, app will not return to workspace editor as the workspace link is invalid.

Actual Result:

In Step 4, app returns to workspace editor with the error "Invalid Policy ID".

Workaround:

Unknown

Platforms:

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

• [x] Android: Native
• [ ] Android: mWeb Chrome
• [x] iOS: Native
• [ ] iOS: mWeb Safari
• [ ] MacOS: Chrome / Safari
• [ ] MacOS: Desktop

Screenshots/Videos

https://github.com/user-attachments/assets/2bfeab4b-d320-4127-805f-7c7ec2347bbb

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~015e953d4b48a8b7a8
• Upwork Job ID: 1821050583313225827
• Last Price Increase: 2024-09-11
• Automatic offers:
• shubham1206agra | Contributor | 103516762
• dominictb | Contributor | 103942180

Issue Owner

Current Issue Owner: @thesahindia

[melvin-bot[bot]]

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

[izarutskaya]

We think this issue might be related to the #collect project.

[stephanieelliott]

This seems like this may be a regression from https://github.com/Expensify/App/issues/46183 -- posted there to have the PR author take a look.

[bernhardoj]

This issue is present before the PR.

[stephanieelliott]

Thanks @bernhardoj -- we'll carry on and treat this as a standalone issue then.

[melvin-bot[bot]]

Job added to Upwork: https://www.upwork.com/jobs/~015e953d4b48a8b7a8

[melvin-bot[bot]]

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

[dominictb]

Proposal

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

In Step 4, app returns to workspace editor with the error "Invalid Policy ID".

What is the root cause of that problem?

In https://github.com/Expensify/App/blob/2c60fc937881a77062f0e07a405fa0380cc847b3/src/libs/Navigation/AppNavigator/createCustomFullScreenNavigator/CustomFullScreenRouter.tsx#L16, we always push the WORKSPACE.INITIAL page into the route if it does not exist yet and we're accessing a workspace page.

So even when the workspace does not exist/is not accessible, we still do that, so when the user swipe from the not found more feature page, they will see the WORKSPACE.INITIAL page with the error.

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

In https://github.com/Expensify/App/blob/2c60fc937881a77062f0e07a405fa0380cc847b3/src/libs/Navigation/AppNavigator/createCustomFullScreenNavigator/CustomFullScreenRouter.tsx#L16, only push the WORKSPACE.INITIAL page into the route if the workspace is valid and accessible.

The condition for "workspace is valid and accessible" that is checked by AccessOrNotFoundWrapper is here

So we can add to this https://github.com/Expensify/App/blob/2c60fc937881a77062f0e07a405fa0380cc847b3/src/libs/Navigation/AppNavigator/createCustomFullScreenNavigator/CustomFullScreenRouter.tsx#L17

const policy = PolicyUtils.getPolicy(workspaceCentralPane?.params?.policyID);
const isPolicyNotAccessible = isEmptyObject(policy) || (Object.keys(policy).length === 1 && !isEmptyObject(policy.errors)) || !policy?.id;
if (isPolicyNotAccessible) {
    return;
}

What alternative solutions did you explore? (Optional)

There could be a chance that when deeplinking, the above condition is evaluated before the policy data is loaded. To prevent false positive in that case we can add a check for isLoadingReportData like we did in https://github.com/Expensify/App/blob/2c60fc937881a77062f0e07a405fa0380cc847b3/src/pages/workspace/withPolicyAndFullscreenLoading.tsx#L33, so we only do not push workspace initial page to the route if the policy is not accessible and isLoadingReportData is false.

Then in AccessOrNotFoundWrapper if we detect that the policy is not found, we should dispatch to remove the workspace initial page from the navigation state. Pseudo-code:

navigationRef.dispatch((state) => {
  const routes = state.routes?.filter(item => item.name !== SCREENS.WORKSPACE.INITIAL);

  return CommonActions.reset({
    ...state,
    routes,
    index: routes.length < state.routes.length ? state.index - 1 : state.index,
  });
});

[shubham1206agra]

Taking over here on request here. https://expensify.slack.com/archives/C02NK2DQWUX/p1723190106955179

@dominictb Can you post a test branch here?

[dominictb]

@shubham1206agra Of course, the test branch is https://github.com/dominictb/epsf-app/tree/fix/46646-test

Looking forward to your review 🙏

[shubham1206agra]

@dominictb Your proposal works, but it looks unstable, as you have mentioned already. Your alternative solution cannot be adopted as we should not use navigationRef.dispatch in the component.

[melvin-bot[bot]]

@stephanieelliott, @thesahindia Huh... This is 4 days overdue. Who can take care of this?

[melvin-bot[bot]]

📣 @shubham1206agra 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

[melvin-bot[bot]]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[dominictb]

@dominictb Your proposal works, but it looks unstable, as you have mentioned already. Your alternative solution cannot be adopted as we should not use navigationRef.dispatch in the component.

I'll give an update on this tomorrow

[melvin-bot[bot]]

@stephanieelliott @shubham1206agra this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[stephanieelliott]

Not overdue, still waiting for proposals

[dominictb]

@dominictb Your proposal works, but it looks unstable, as you have mentioned already. Your alternative solution cannot be adopted as we should not use navigationRef.dispatch in the component.

Sorry 🙏 last week I wasn't able to get down to this, I'll post an update by tomorrow

[melvin-bot[bot]]

@stephanieelliott, @shubham1206agra Whoops! This issue is 2 days overdue. Let's get this updated quick!

[dominictb]

@dominictb Your proposal works, but it looks unstable, as you have mentioned already

@shubham1206agra I thought so as a potential enhancement, but there's actually no problem with the main solution. I tested with multiple scenarios of deep linking and they all work correctly.

Did you find any scenario where the solution does not work? If not I think we can move forward with it.

[melvin-bot[bot]]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[melvin-bot[bot]]

@stephanieelliott, @shubham1206agra Huh... This is 4 days overdue. Who can take care of this?

[melvin-bot[bot]]

@stephanieelliott, @shubham1206agra Still overdue 6 days?! Let's take care of this!

[shubham1206agra]

Waiting for proposals

[dominictb]

Did you find any scenario where the solution does not work? If not I think we can move forward with it.

@shubham1206agra What do you think? We already have a working solution here.

[shubham1206agra]

Did you find any scenario where the solution does not work? If not I think we can move forward with it.

@shubham1206agra What do you think? We already have a working solution here.

Not really, but I want a stable solution for this.

[shubham1206agra]

@adamgrzybowski Can you help us here with your expertise?

[melvin-bot[bot]]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[melvin-bot[bot]]

@stephanieelliott @shubham1206agra this issue is now 4 weeks old, please consider:

• Finding a contributor to fix the bug
• Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
• If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

[stephanieelliott]

Bump @adamgrzybowski -- any chance you can help us on this one?

[adamgrzybowski]

I am currently on vacations. I will be back after september 10.

[wildan-m]

Proposal

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

Workspace app redirects to the workspace editor with an error after returning from a different page.

What is the root cause of that problem?

Native swipe back is not navigated to desired place. When FullPageNotFoundView shown in workspace pages, we should navigate to workspace, it's wrapped in PolicyUtils.goBackFromInvalidPolicy function.

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

In Android, we can customize back button/swipe behavior by using BackHandler.addEventListener('hardwareBackPress' and overriding the back function with onBackButtonPress in FullPageNotFoundView.

src/components/BlockingViews/FullPageNotFoundView.tsx

    // To block android native back button behavior
    useFocusEffect(
        useCallback(() => {
            if (!onBackButtonPress) {
                return;
            }

            const backHandler = BackHandler.addEventListener('hardwareBackPress', () => {
                onBackButtonPress();
                return true;
            });

            return () => {
                backHandler.remove();
            };
        }, [onBackButtonPress]),
    );

IOS hardwareBackPress only supported in Android, but in iOS I've noticed that if we disable gesture in the last screen in the same navigator, then previous screen swipe action will be executed. So, if the not found page shown, we can disable all gestures in workspace screens, except the first screen in the route stack.

    const navigation = useNavigation();

    // Handle back swipe on iOS
    useLayoutEffect(() => {
        if (!shouldShow) {
            return;
        }

        const state = navigation.getState();
        if (!state) {
            return;
        }

        // Check if the last route's name is 'not-found' or if there is no parent navigator
        const lastRoute = state.routes[state.routes.length - 1];
        if (lastRoute.name === SCREENS.NOT_FOUND || !navigation.getParent()) {
            return;
        }

        // Check if the first route's name is not the initial workspace screen
        // Currently we only have case for workspace, this check to avoid potential regression
        if (state.routes[0].name !== SCREENS.WORKSPACE.INITIAL) {
            return;
        }

        // Iterate through all routes and disable gestures for all except the first one
        state.routes.forEach((route, index) => {
            navigation.setOptions({
                gestureEnabled: index === 0,
            });
        });
    }, [navigation]);

I placed the code in FullPageNotFoundView.tsx without any problems. If needed, we can create platform-specific code for safety.

Branch for this solution.

What alternative solutions did you explore? (Optional)

N/A s

[dominictb]

Your alternative solution cannot be adopted as we should not use navigationRef.dispatch in the component.

@shubham1206agra We can place the navigationRef.dispatch in a removeScreenFromNavigationState method and use it.

Other places that we use navigationRef.dispatch, we use the same way (i.e. here and here, both those methods use navigationRef.dispatch within it)

@shubham1206agra What do you think?

[melvin-bot[bot]]

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

[wildan-m]

@shubham1206agra any opinion about my proposal?

https://github.com/Expensify/App/issues/46646#issuecomment-2320353524

[melvin-bot[bot]]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[melvin-bot[bot]]

@stephanieelliott, @shubham1206agra Huh... This is 4 days overdue. Who can take care of this?

[stephanieelliott]

Hey @shubham1206agra can you review the proposal here please? https://github.com/Expensify/App/issues/46646#issuecomment-2320353524

[dominictb]

@shubham1206agra Gentle bump on this thanks

[shubham1206agra]

I need to consult someone who is an expert in the Navigation code.

Since I am not a fan of any proposals right now.

[dominictb]

Since I am not a fan of any proposals right now.

@shubham1206agra Could you give a bit more details on why this is? FYI I had an update here that explains why my alternative solution can be used and it won't have any edge case that you're worried about.

[shubham1206agra]

Since I am not a fan of any proposals right now.

@shubham1206agra Could you give a bit more details on why this is? FYI I had an update here that explains why my alternative solution can be used and it won't have any edge case that you're worried about.

It's not why it's more the solution requires us to go through every screen currently mounted which I don't like.

[dominictb]

why it's more the solution requires us to go through every screen currently mounted

@shubham1206agra From what I can see, the only place we need to apply the solution is the AccessOrNotFoundWrapper one, so what do you mean by "every screen currently mounted"?

[wildan-m]

@shubham1206agra you might want to call CME to discuss your concern and decide whether to move with current proposal or do another option.

[melvin-bot[bot]]

@stephanieelliott, @shubham1206agra Eep! 4 days overdue now. Issues have feelings too...

[stephanieelliott]

I need to consult someone who is an expert in the Navigation code.

Hey @shubham1206agra, were you able to do this yet? If no, maybeyou start a Slack thread so we can chat through your concerns? That will let us pull in a wider audience so we figure out next steps here

[mountiny]

I have reached out to @adamgrzybowski here

[melvin-bot[bot]]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[adamgrzybowski]

Hey, I did some testing, and the best solution I figured out is the same as the alternative solution proposed by @dominictb.

I like the alternative more because we are assuming the happy path which would occur in most cases. Also, I think removing one route sounds like a less invasive operation than adding one with delay.

I know it's not beautiful but combining navigation and checking the correctness of the policyID leaves us little room for other solutions.

I would be happy to review the final solutions though to make sure we don't miss any edge cases.

Also given @shubham1206agra concerns regarding stability, we wouldn't need to make sure that the reset action is called once.

cc: @mountiny

[shubham1206agra]

Thanks for the help @adamgrzybowski

Lets go with @dominictb's proposal in this case.

🎀👀🎀 C+ reviewed