[$250] Workspace list is opened in background when workspace description is opened from workspace chat
If you havenβt already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
Version Number: 9.0.72-0 Reproducible in staging?: Yes Reproducible in production?: Yes If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: N/A If this was caught during regression testing, add the test name, ID and link from TestRail: Exp Email or phone of affected tester (no customers): [email protected] Issue reported by: Applause Internal Team
Action Performed:
- Go to staging.new.expensify.com
- Create a workspace.
- Go to workspace settings > Profile.
- Click Description, add a description and save it.
- Click on the workspace chat under "Submit expenses using your workspace chat below" on the same page (important).
- In workspace chat, click on the header subtitle to open workspace description editor.
Expected Result:
The background will be workspace settings.
Actual Result:
The background is workspace list.
Workaround:
Unknown
Platforms:
- [ ] Android: Standalone
- [ ] Android: HybridApp
- [ ] Android: mWeb Chrome
- [ ] iOS: Standalone
- [ ] iOS: HybridApp
- [ ] iOS: mWeb Safari
- [x] MacOS: Chrome / Safari
- [x] MacOS: Desktop
Screenshots/Videos
https://github.com/user-attachments/assets/afaedc1c-3380-47b2-9958-876a3c488ff5
Upwork Automation - Do Not Edit
- Upwork Job URL: https://www.upwork.com/jobs/~021864885429995006456
- Upwork Job ID: 1864885429995006456
- Last Price Increase: 2024-12-06
- Automatic offers:
- twilight2294 | Contributor | 105273277
Issue Owner
Current Issue Owner: @dukenv0307
Triggered auto assignment to @kadiealexander (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.
Edited by proposal-police: This proposal was edited at 2024-12-10 04:12:50 UTC.
Proposal
Please re-state the problem that we are trying to solve in this issue.
When user clicks on the workspace description in the workspace header, The workspaces list is shown in the background, but Workspace profile is expected.
What is the root cause of that problem?
We are not passing backTo here, when user click on the desc: https://github.com/Expensify/App/blob/840c227be7fd907ab096cda7d6f28e1eef4a1d30/src/pages/home/HeaderView.tsx#L265
What changes do you think we should make in order to solve the problem?
pass ROUTES.WORKSPACE_INITIAL.getRoute(report?.policyID ?? '-1') as the backTo route:
Navigation.navigate(ROUTES.WORKSPACE_PROFILE_DESCRIPTION.getRoute(report.policyID ?? '-1', ROUTES.WORKSPACE_INITIAL.getRoute(report?.policyID ?? '-1')));
and modify the ROUTES.WORKSPACE_PROFILE_DESCRIPTION.getRoute to accept backTo :
https://github.com/Expensify/App/blob/840c227be7fd907ab096cda7d6f28e1eef4a1d30/src/ROUTES.ts#L856
as:
getRoute: (policyID: string, backTo?: string) => getUrlWithBackToParam(`settings/workspaces/${policyID}/profile/description` as const, backTo),
Job added to Upwork: https://www.upwork.com/jobs/~021864885429995006456
Triggered auto assignment to Contributor-plus team member for initial proposal review - @dukenv0307 (External)
Edited by proposal-police: This proposal was edited at 2024-12-06 09:09:03 UTC.
Proposal
Please re-state the problem that we are trying to solve in this issue.
Workspace list is opened in background when workspace description is opened from workspace chat
What is the root cause of that problem?
The issue occurs because the Report route is included in the navigation stack after clicking the workspace chat under βSubmit expenses using your workspace chat below.β Additionally, when the header subtitle is clicked, navigation to WORKSPACE_PROFILE_DESCRIPTION is triggered, which then adds both the Settings_Workspaces route and the RightModalNavigator to the stack.
Here is log for this:
BottomTabNavigator -> Settings_Workspace -> FullScreenNavigator -> Report -> Settings_Workspaces -> RightModalNavigator
This results in an incorrect navigation flow, as the Report route is stacked before Settings_Workspaces, and FullScreenNavigator is stacked before Report. This causes navigation to the workspace list instead of the workspace profile, as the workspace profile is located in FullScreenNavigator.
What changes do you think we should make in order to solve the problem?
To resolve this issue, we need to remove the Report route when navigating to WORKSPACE_PROFILE_DESCRIPTION.
BottomTabNavigator -> Settings_Workspace -> FullScreenNavigator -> Settings_Workspaces -> RightModalNavigator
To do this, we need to add Navigation.goBack() to return to the FullScreenNavigator before navigating to WORKSPACE_PROFILE_DESCRIPTION. Something like this:
1 Add go back before navigation to WORKSPACE_PROFILE_DESCRIPTION
https://github.com/Expensify/App/blob/5180ef4b9b47a48dda6e4d35e3db615cf70aa187/src/pages/home/HeaderView.tsx#L264-L267
if (ReportUtils.canEditPolicyDescription(policy)) {
Navigation.goBack();
Navigation.navigate(ROUTES.WORKSPACE_PROFILE_DESCRIPTION.getRoute(report.policyID ?? '-1'));
return;
}
2 This is similar to this issue; we still applied goBack before navigating to WORKSPACE_PROFILE_DESCRIPTION
https://github.com/Expensify/App/blob/e8dd2bd3496a313f378678028c2a7b62c0acf30b/src/components/ReportWelcomeText.tsx#L109-L114
onPress={() => {
if (!canEditPolicyDescription) {
return;
}
Navigation.goBack();
Navigation.navigate(ROUTES.WORKSPACE_PROFILE_DESCRIPTION.getRoute(policy?.id ?? '-1'));
}}
POC
https://github.com/user-attachments/assets/7130ea56-f1bc-4f77-9df5-1dd7d2fd3039
What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?
What alternative solutions did you explore? (Optional)
Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.
Proposal
Please re-state the problem that we are trying to solve in this issue.
Workspace list is opened in background when workspace description is opened from workspace chat
What is the root cause of that problem?
When we click on workspace chat in policy settings, FullScreenNavigator is pushed on the screen navigation stack and not replaced:
https://github.com/Expensify/App/blob/3baf965618ced48845ddd7534b0fad4d4110b5de/src/libs/Navigation/linkTo/index.ts#L114-L118
This issue is only caused on the profile tabs and not on other tabs of the workspace because profile tab is at the root of the workspace settings screen.
What changes do you think we should make in order to solve the problem?
We should pass CONST.NAVIGATION.TYPE.UP in here while navigating to workspace chat this will make sure that we replace the appropriate screen at the root:
<MenuItem
title={getReportName(currentUserPolicyExpenseChat)}
description={translate('workspace.common.workspace')}
icon={getIcons(currentUserPolicyExpenseChat, personalDetails)}
onPress={() => Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(currentUserPolicyExpenseChat?.reportID ?? '-1'), CONST.NAVIGATION.TYPE.UP)}
shouldShowRightIcon
wrapperStyle={[styles.br2, styles.pl2, styles.pr0, styles.pv3, styles.mt1, styles.alignItemsCenter]}
shouldShowSubscriptAvatar
/>
What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?
N/A navigation bug so no tests
What alternative solutions did you explore? (Optional)
Proposal
Please re-state the problem that we are trying to solve in this issue.
Workspace list is opened in background when workspace description is opened from workspace chat
What is the root cause of that problem?
The issue occurs because the navigation logic uses Navigation.navigate() for the route, which may reuse a cached navigation stack containing the workspace list. This happens when the navigation state isn't cleanly reset or managed.
What changes do you think we should make in order to solve the problem?
Replace Navigation.navigate() with Navigation.closeAndNavigate() to ensure the navigation state is cleaned and the correct route is opened without reusing unintended screens.
Code Change in HeaderView.tsx lines 263 - 269 should look like this:
onPress={() => {
if (ReportUtils.canEditPolicyDescription(policy)) {
Navigation.closeAndNavigate(ROUTES.WORKSPACE_PROFILE_DESCRIPTION.getRoute(report.policyID ?? '-1'));
return;
}
Navigation.closeAndNavigate(ROUTES.REPORT_WITH_ID_DETAILS.getRoute(reportID, Navigation.getReportRHPActiveRoute()));
}}
https://github.com/user-attachments/assets/007fb0f5-e8d1-4c80-a1eb-d1233c1b9c17
What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?
What alternative solutions did you explore? (Optional)
Alternative you can use a replace function, resetting or a conditional goBack.
π£ @skanderphilipp! π£ 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:
- Make sure you've read and understood the contributing guidelines.
- 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.
- 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.
- 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>
@skanderphilipp Your proposal will be dismissed because you did not follow the proposal template.
Contributor details Your Expensify account email: [email protected] Upwork Profile Link: Upwork Profile
β Contributor details stored successfully. Thank you for contributing to Expensify!
@huult @twilight2294 Can you please elaborate on the RCA, they don't look enough to me
And we click in HeaderView to go to WORKSPACE_PROFILE_DESCRIPTION then Report will pushed to pushed root state.
When we click on workspace chat in policy settings, FullScreenNavigator is pushed on the screen navigation stack and not replaced:
@dukenv0307, What are your thoughts on my proposal? Passing the backTo route is a common pattern we use throughout the app, and it seems to align well with the expected behavior.
Currently, the backTo route is passed in other cases on the same page but is missing specifically for the workspace description. For example, we are passing backTo for reportDescription, but not for workspaceDescription. This inconsistency seems to be the root cause of the issue, Thanks.
https://github.com/Expensify/App/blob/840c227be7fd907ab096cda7d6f28e1eef4a1d30/src/pages/home/HeaderView.tsx#L244-L245
@Shahidullah-Muffakir Can you elaborate on the RCA? And I did test your solution and it didn't work.
@dukenv0307 Thank you for reviewing my proposal. I will update the RCA now
@Shahidullah-Muffakir Can you elaborate on the RCA? And I did test your solution and it didn't work.
@dukenv0307, we need to pass the backTo route here because:
-
In other parts of the app when RHP open, we pass the backTo route to ensure the current page is show as the fullscreen navigator in the bacgrkound and when user press the back button the user redirects to the current page, without it, the app defaults to the FullScreenNavigator, if user press the back button.
-
The backTo route is also needed when the user refreshes the workspace description edit page. Without it, the page falls back to the workspace list in the background instead of reopening with the Workspace Settings page in the background
The changes I added:
- modified this: https://github.com/Expensify/App/blob/840c227be7fd907ab096cda7d6f28e1eef4a1d30/src/ROUTES.ts#L856 as: getRoute: (policyID: string, backTo?: string) => getUrlWithBackToParam(`settings/workspaces/${policyID}/profile/description` as const, backTo)
and:
2.modified this https://github.com/Expensify/App/blob/840c227be7fd907ab096cda7d6f28e1eef4a1d30/src/pages/home/HeaderView.tsx#L265
as:
Navigation.navigate(ROUTES.WORKSPACE_PROFILE_DESCRIPTION.getRoute(report.policyID ?? '-1', Navigation.getActiveRoute()));
After adding my changes:
https://github.com/user-attachments/assets/87560a9f-8df2-4d88-bd88-aa558881902f
@skanderphilipp Your proposal will be dismissed because you did not follow the proposal template.
I updated the proposal, is there still some issues with the template ?
Proposal Please re-state the problem that we are trying to solve in this issue. Workspaces list is opened in background when workspace description is opened from workspace chat under "Submit expenses using your workspace chat below"
What is the root cause of that problem? The issue occur due to preserve the current screen in the navigation stack. Commonly occurs when we need the current screen using Navigation.navigate() in stack.
What changes do you think we should make in order to solve the problem? To resolve the issue we need to add a Navigation.goBack() before navigating to other screen so that we can clear the current screen as per stack to show the workspace settings screen.
Code Change in HeaderView.tsx lines 263 - 270 should look like this:
onPress={() => { if (ReportUtils.canEditPolicyDescription(policy)) { Navigation.goBack(); Navigation.navigate(ROUTES.WORKSPACE_PROFILE_DESCRIPTION.getRoute(report.policyID ?? '-1')); return; } Navigation.navigate(ROUTES.REPORT_WITH_ID_DETAILS.getRoute(reportID, Navigation.getReportRHPActiveRoute())); }}
After implementing the above changes in code:
https://github.com/user-attachments/assets/9ee1302c-4361-4bba-9c32-b0e3aabdce68
What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?
What alternative solutions did you explore? (Optional) Alternatives can be replace() to use the required screen in the stack or reset() to clear the stack.
π£ @Percept-Systems! π£ 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:
- Make sure you've read and understood the contributing guidelines.
- 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.
- 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.
- 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>
Contributor details Your Expensify account email: [email protected] Upwork Profile Link: https://www.upwork.com/freelancers/~017dcb7ae0dadbc660
β Contributor details stored successfully. Thank you for contributing to Expensify!
Proposal Updated
- Add new RCA.
Short RCA: The issue occurs due to an incorrect navigation stack: Report is before Settings_Workspaces, and FullScreenNavigator is before Report, causing navigation to the workspace list instead of the workspace profile.
Solution: We need to use goBack to remove the Report route and ensure the navigation works as expected
@dukenv0307 Iβve updated the new RCA. If itβs still not enough for you, please let me know, or if you have any questions. Thank you!
@Shahidullah-Muffakir Your evidence is incorrect, our expectation here is The background will be workspace settings, not report chat
@huult @Percept-Systems We shouldn't use goBack since we can't guarantee the latest route is Report. Pls check the video below:
https://github.com/user-attachments/assets/8bdd38c4-284c-464c-ac47-9ad8727b170d
Triggered auto assignment to @nkuoch, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
@Shahidullah-Muffakir Your evidence is incorrect, our expectation here is
The background will be workspace settings, not report chat
@dukenv0307, thanks for clarifying, I misunderstood the expected behavior.
Instead of passing the current page as backTo, we can pass the workspace's initial page. Here's the updated code: Navigation.navigate(ROUTES.WORKSPACE_PROFILE_DESCRIPTION.getRoute(report.policyID ?? '-1', ROUTES.WORKSPACE_INITIAL.getRoute(report?.policyID ?? '-1')));
Updated the proposal https://github.com/Expensify/App/issues/53665#issuecomment-2521701789 as well
https://github.com/user-attachments/assets/55fd88cf-2c53-4237-acd9-742f1a9582f9
π£ @twilight2294 π 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 π
@twilight2294 any updates?
PR ready for review @dukenv0307