[HOLD for payment 2024-10-30] [$250] mWeb - Workspace - Opens "Hmm it's not here" when tapping system back button

[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.40
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/5007250
Issue reported by: Applause - Internal Team

Action Performed:

Employee's side

1. Log in to the app as a workspace employee.
2. Navigate to Workspace settings > Profile
3. Tap on the system back button
4. Tap on Members (or Profile again)
5. Tap on the system back button
   Admin's side
6. Log in to the app as a workspace admin.
7. Navigate to Workspace settings
8. Tap on any setting, for instance, Workflows
9. Tap on the system back button
10. Tap on any other setting (or the same tapped in step 3)
11. Tap on the system back button

Expected Result:

The user returns to the Workspace settings page

Actual Result:

The "Hmm... it's not here" page is displayed:
Hmm... it's not here
Oops, this page cannot be found

Workaround:

Unknown

Platforms:

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

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6617565_1727454588873.it_s_not_here-admin_s_side.mp4.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021843764334226859970
• Upwork Job ID: 1843764334226859970
• Last Price Increase: 2024-10-15
• Automatic offers:
• c3024 | Contributor | 104469646

Issue Owner

Current Issue Owner: @jliexpensify

[melvin-bot]

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

[lanitochka17]

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

[jliexpensify]

I can't repro this on 9.0.41-2 (Pixel 6)

[lanitochka17]

Issue is still reproducible on the latest build 9.0.44-10
These steps should help you reproduce the issue:

1. Go to settings click on workspaces and open one
2. Click on members
3. Click on 3 dots at top right corner
4. Click phone back button
5. Observe user is taken to workspace detail page then click members again
6. Click on 3 dots again and then click on the phones back button
7. Observe that hmm not found page is shown

Bug6618542_1727536595923.Screen_Recording_20240928_180655.mp4

[c3024]

Edited by proposal-police: This proposal was edited at 2024-10-05 17:48:57 UTC.

Proposal

Please restate the problem that we are trying to solve in this issue.

Navigating back with the device back button disrupts the URL when navigating from pages like “Members” and “Categories" etc. on narrow layouts.

What is the root cause of that problem?

We normalize a path by adding '/' to it if it does not exist.

App/src/libs/Navigation/linkingConfig/getAdaptedStateFromPath.ts

Line 369 in 5cf3d19

const normalizedPath = !path.startsWith('/') ? `/${path}` : path;

We remove policyID from it and find the state from this normalizedPath but when shouldReplacePathInNestedState is true we replace the focusedRoute with the initial path (that is the path that might not have '/')

App/src/libs/Navigation/linkingConfig/getAdaptedStateFromPath.ts

Lines 370 to 379 in 5cf3d19

	const pathWithoutPolicyID = getPathWithoutPolicyID(normalizedPath);
	const isAnonymous = isAnonymousUser();
	// Anonymous users don't have access to workspaces
	const policyID = isAnonymous ? undefined : extractPolicyIDFromPath(path);
	const state = getStateFromPath(pathWithoutPolicyID, options) as PartialState<NavigationState<RootStackParamList>>;
	if (shouldReplacePathInNestedState) {
	replacePathInNestedState(state, path);
	}

So, here, route path /settings/workspaces/:policyID the state is replaced with settings/workspaces/:policyID.

This causes an inconsistency with the browser location path /settings/workspaces/:policyID with that in the navigation state. This causes the browser to go back once more and the URL gets deformed.

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

We can pass the normalizedPath to replacePathInNestedState instead of path here

App/src/libs/Navigation/linkingConfig/getAdaptedStateFromPath.ts

Line 378 in 5cf3d19

replacePathInNestedState(state, path);

What alternative solutions did you explore? (Optional)

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[ntdiary]

During the review, the inspection of the above proposal may take another day. 🤔

[ntdiary]

Hi, @adamgrzybowski, do you remember which specific issue the replacePathInNestedState function was meant to address? I only found that you added this function, but couldn't find any related issues, would appreciate if you could provide more context. ❤️
PR: #33280 (comment)

When we open a workspace from the WorkspaceListPage, replacePathInNestedState replaces the path in the state with a path that doesn’t have a / prefix, which is inconsistent with the path stored in memory history. This seems to have caused the url not to display correctly if we open the members page and then click the browser's back button.

[c3024]

Oh, for some reason I misread the path here

App/src/libs/Navigation/linkingConfig/getAdaptedStateFromPath.ts

Line 378 in 9d86f30

replacePathInNestedState(state, path);

as normalizedPath.

So, I could not identify where the '/' was being removed.

[ntdiary]

Oh, for some reason I misread the path here

App/src/libs/Navigation/linkingConfig/getAdaptedStateFromPath.ts

Line 378 in 9d86f30

replacePathInNestedState(state, path);

as normalizedPath.
So, I could not identify where the '/' was being removed.

I'm still digging in and verifying. 😄

[melvin-bot]

@ntdiary @jliexpensify 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!

[adamgrzybowski]

Hey @ntdiary, sorry for the delay. Unfortunately, I don't remember the exact case. It was probably something like going back (with the in app button) to this page causing a wrong path in the browser's URL bar if the path wasn't corrected in the state.

[ntdiary]

Hey @ntdiary, sorry for the delay. Unfortunately, I don't remember the exact case. It was probably something like going back (with the in app button) to this page causing a wrong path in the browser's URL bar if the path wasn't corrected in the state.

@adamgrzybowski, thank you! :)

[ntdiary]

I have not found any other cases where getPathFromState does not return the correct path. The getPathWithoutPolicyID functions also check with a regex in the format of /w/, which I have not found anywhere in ROUTES. So, we might consider removing this function replacePathInNestedState completely.

@c3024, we have a workspace switcher, and the /w URL is used when switching workspaces.

Based on my investigation, the issue seems more related to a mismatch between two paths in the memory history.

Instead of having replacePathInNestedState return a new state, perhaps I’m leaning more toward avoiding the use of replacePathInNestedState, maybe change something like this:

App/src/libs/Navigation/linkTo/index.ts

Line 136 in 8ab4792

const {adaptedState, metainfo} = getAdaptedStateFromPath(path, linkingConfig.config);

const {adaptedState, metainfo} = getAdaptedStateFromPath(path, linkingConfig.config, false);

Additionally, I’m working on testing more accurate and comprehensive cases, so that we can move forward with more confidence. :)

[melvin-bot]

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

[ntdiary]

@c3024, can you please update the RCA in your proposal, so we can move forward with it? This screenshot might help:

the path (in the state) and pendingPath (i.e., location.path + location.search ) are inconsistent, causing navigation to call history.go(-1) one extra time, which leads to an abnormal URL.

As for possible implementations, I think there are two options:

1. Use replacePathInNestedState(state, normalizedPath);
2. Completely remove the replacePathInNestedState function.

So far, I haven't noticed any regressions with these approach, but I recommend the first one because it involves fewer changes and will likely have a smaller impact. Please feel free to let me know if you have any different ideas. :)

[c3024]

Updated, thank you very much 🙇 !

Hopefully, I’ll become a Chrome DevTools wizard like you someday! 😆

[ntdiary]

Updated, thank you very much 🙇 !

Nice! Based on my prev summary, I think it's fine to move forward with @c3024's proposal. :)

🎀 👀 🎀 C+ reviewed

[melvin-bot]

Triggered auto assignment to @marcochavezf, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[marcochavezf]

Thanks for the review @ntdiary, assigning @c3024 🚀

[melvin-bot]

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

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

[melvin-bot]

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

• Replace with correct path in nested state #51071

If no regressions arise, payment will be issued on 2024-10-30. 🎊

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

• @ntdiary requires payment through NewDot Manual Requests
• @c3024 requires payment automatic offer (Contributor)

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@ntdiary / @c3024] The PR that introduced the bug has been identified. Link to the PR:
• [@ntdiary / @c3024] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@ntdiary / @c3024] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@ntdiary / @c3024] Determine if we should create a regression test for this bug.
• [@ntdiary / @c3024] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@jliexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[ntdiary]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@ntdiary / @c3024] The PR that introduced the bug has been identified. Link to the PR: [Wave 8] Ideal nav  #33280
• [@ntdiary / @c3024] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: https://github.com/Expensify/App/pull/33280/files#r1823739651
• [@ntdiary / @c3024] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: No need
• [@ntdiary / @c3024] Determine if we should create a regression test for this bug. Yes
• [@ntdiary / @c3024] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again. Shown as below
• [@jliexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

Since I've seen quite a few issues related to navigation, I think it's fine to create a regression test for this issue.

Regression Test Proposal
This issue only occurs on mobile web platforms.

1. Log in to the ND using an account with a workspace.
2. Click on "Settings" and then click on "Workspaces".
3. Select a workspace.
4. remember its URL.
5. open its Members page.
6. Click the browser/device back button.
7. Verify the URL is same as step 4.
8. open its Members page and click the browser/device back button again.
9. Verify "Hmm... it's not here" page won't display and the URL is still same as step 4.
10. Do we agree 👍 or 👎

[jliexpensify]

Payment Summary

• C: @c3024 $250 (paid via Upworks already
• C+: @ntdiary $250 (needs to be paid via New.Dot)

[JmillsExpensify]

$250 approved for @ntdiary