-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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.
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
[Due for payment 2025-03-11] [$250] [Navigation] Browser Forward Button Fails to Restore Policy Filter After Navigation #57121
Comments
Triggered auto assignment to @zanyrenney ( |
@mountiny I noticed in the related issue that is linked above you said that softwaremansion and we're looking into some of these issues, just to confirm before I add the external label can this be handled by our normal bug process or should it be assigned to software mansion? |
Thanks for the ping, I agree this will be best checked by SWM |
I would suggest to handle it this way: diff --git a/src/pages/home/sidebar/BaseSidebarScreen.tsx b/src/pages/home/sidebar/BaseSidebarScreen.tsx
index 369d1f23459..2392a12d508 100644
--- a/src/pages/home/sidebar/BaseSidebarScreen.tsx
+++ b/src/pages/home/sidebar/BaseSidebarScreen.tsx
@@ -39,20 +39,17 @@ function BaseSidebarScreen() {
}, []);
useEffect(() => {
- if (!!activeWorkspace || activeWorkspaceID === undefined || isLoading) {
- return;
- }
-
- // Otherwise, if the workspace is already loaded, we don't need to do anything
- const topmostReport = getTopmostReportsSplitNavigator();
+ const topmostReportSplit = getTopmostReportsSplitNavigator();
+ const topmostReportSplitPolicyID = topmostReportSplit?.params && `policyID` in topmostReportSplit.params ? topmostReportSplit.params.policyID : undefined;
+ const hasWorkspaceBeenDeleted = !activeWorkspace && activeWorkspaceResult.status === 'loaded' && !!topmostReportSplitPolicyID;
- if (!topmostReport) {
+ if (!topmostReportSplit || !hasWorkspaceBeenDeleted || isLoading) {
return;
}
// Switching workspace to global should only be performed from the currently opened sidebar screen
- const topmostReportState = topmostReport?.state ?? getPreservedSplitNavigatorState(topmostReport?.key);
- const isCurrentSidebar = topmostReportState?.routes.some((route) => currentRoute.key === route.key);
+ const topmostReportSplitState = topmostReportSplit?.state ?? getPreservedSplitNavigatorState(topmostReportSplit?.key);
+ const isCurrentSidebar = topmostReportSplitState?.routes.some((route) => currentRoute.key === route.key);
if (!isCurrentSidebar) {
return;
@@ -61,7 +58,7 @@ function BaseSidebarScreen() {
const reportsSplitNavigatorWithoutPolicyID = getInitialSplitNavigatorState({name: SCREENS.HOME}, {name: SCREENS.REPORT});
Navigation.replaceWithSplitNavigator(reportsSplitNavigatorWithoutPolicyID);
updateLastAccessedWorkspace(undefined);
- }, [activeWorkspace, activeWorkspaceID, isLoading, currentRoute.key]);
+ }, [activeWorkspace, isLoading, currentRoute.key, activeWorkspaceResult]);
const shouldDisplaySearch = shouldUseNarrowLayout; The code at the top of this hook could have been written in a better way, but I wanted to see if it works asap. I also tested how it behaves after deleting the policy and it seems to work well. When we read a policyID directly from the state, it's faster than reading it from the context. Here's a video: Screen.Recording.2025-02-20.at.16.29.19.mov |
Job added to Upwork: https://www.upwork.com/jobs/~021892616298138120650 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @parasharrajat ( |
Thanks for the proposal @FitseTLT I will let SWM handle this though as they have a good context on the overall navigation structure of the app |
@FitseTLT I think you should finish the pending jobs before finding the new one. |
SWM 🎀 👀 🎀 C+ reviewed |
Current assignee @mountiny is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new. |
Hello, I can take care of this one 🙋♀️ |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 9.1.8-1 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2025-03-11. 🎊 For reference, here are some details about the assignees on this issue:
|
@parasharrajat @zanyrenney @parasharrajat The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button] |
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.1.1-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?:
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: @s77rt
Slack conversation (hyperlinked to channel name): #Expensify Bugs
Action Performed:
Expected Result:
User taken to Policy filter selected in step 1
Actual Result:
The home page flickers and the previous filter is not applied
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
Screen.Recording.2025-02-19.at.2.15.50.PM.mov
Recording.999.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @zanyrenneyIssue Owner
Current Issue Owner: @WojtekBomanThe text was updated successfully, but these errors were encountered: