-
Notifications
You must be signed in to change notification settings - Fork 3.5k
fix: WS - Not here page appears when trying to navigate to Workspaces. #62747
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
Conversation
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
@QichenZhu Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid-native.mp4Android: mWeb Chromeandroid-web.mp4iOS: HybridAppios-native.moviOS: mWeb Safariios-web.movMacOS: Chrome / Safarimac-web.movMacOS: Desktopmac-desktop.mov |
|
sorry QichenZhu, please hold on review, I forgot to add the unit tests. |
|
Please ping me when it's ready. |
|
No problem @Krishna2323. Thank you for letting me know. |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
@QichenZhu, the tests have been added. Could you please take a look when you get a chance? |
|
Bug (NAB): Deleting workspace makes other admin see "Hmm... it's not here" while viewing inbox Steps:
Expected result: User B stays on Inbox screen Screen.Recording.2025-06-04.at.19.33.03.mov |
|
Bug (NAB): Deleting workspace makes other member see "Hmm... it's not here" while viewing workspace settings Steps:
Expected result: User B sees the workspaces list screen Screen.Recording.2025-06-04.at.20.07.47.mov |
| switch (navigationAction?.type) { | ||
| case 'goBack': | ||
| if (navigationAction.route) { | ||
| Navigation.goBack(navigationAction.route); | ||
| } | ||
| break; | ||
|
|
||
| let workspacesTabState = lastWorkspacesTabNavigatorRoute.state; | ||
| if (!workspacesTabState && lastWorkspacesTabNavigatorRoute.key) { | ||
| workspacesTabState = getPreservedNavigatorState(lastWorkspacesTabNavigatorRoute.key); | ||
| } | ||
| case 'navigate': | ||
| if (navigationAction.route) { | ||
| Navigation.navigate(navigationAction.route); | ||
| } | ||
| break; | ||
|
|
||
| // If there is a workspace navigator route, then we should open the workspace initial screen as it should be "remembered". | ||
| if (lastWorkspacesTabNavigatorRoute.name === NAVIGATORS.WORKSPACE_SPLIT_NAVIGATOR) { | ||
| const params = workspacesTabState?.routes.at(0)?.params as WorkspaceSplitNavigatorParamList[typeof SCREENS.WORKSPACE.INITIAL]; | ||
| // Screens of this navigator should always have policyID | ||
| if (params.policyID) { | ||
| const workspaceScreenName = !shouldUseNarrowLayout ? getLastVisitedWorkspaceTabScreen() : SCREENS.WORKSPACE.INITIAL; | ||
| // This action will put settings split under the workspace split to make sure that we can swipe back to settings split. | ||
| case 'dispatch': | ||
| if (navigationAction.dispatchType && navigationAction.payload) { | ||
| navigationRef.dispatch({ | ||
| type: CONST.NAVIGATION.ACTION_TYPE.OPEN_WORKSPACE_SPLIT, | ||
| payload: { | ||
| policyID: params.policyID, | ||
| screenName: workspaceScreenName, | ||
| }, | ||
| type: navigationAction.dispatchType, | ||
| payload: navigationAction.payload, | ||
| }); | ||
| } | ||
| return; | ||
| } | ||
| break; | ||
|
|
||
| Navigation.navigate(ROUTES.WORKSPACES_LIST.route); | ||
| }); | ||
| }, [shouldUseNarrowLayout]); | ||
| case 'return': | ||
| default: | ||
| break; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Krishna2323, thanks for the refactor! Could you share why you prefer to return the action from the helper and perform it in the component, rather than doing it directly in the helper?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm updating this, I think there is no reason to return an action from the helper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Just ping me when it's ready.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@QichenZhu, ready for review 😅
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
| const workspaceScreenName = !shouldUseNarrowLayout ? getLastVisitedWorkspaceTabScreen() : SCREENS.WORKSPACE.INITIAL; | ||
| navigationRef.dispatch({type: CONST.NAVIGATION.ACTION_TYPE.OPEN_WORKSPACE_SPLIT, payload: {policyID: params.policyID, screenName: workspaceScreenName}}); | ||
| return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the comment back and removed the return statement. I'm not sure about the formatting cause the updated one is correct according to the prettier rules:
Line 7 in 0e2b131
| printWidth: 190, |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
Update: The blocking PR was reverted, so I'll resume the review today. |
QichenZhu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Krishna2323, it's still possible to see the "not here" page if user B clicks Workspace immediately after user A deletes it. Still, I suggest merging this first since it's a good improvement that fixes the issue in most cases. There might be other causes, and we can keep digging. What do you think? @neil-marcellini
Screen.Recording.2025-06-12.at.20.53.54.mov
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
@Krishna2323 I'm having trouble understanding these changes. Please go through the whole PR again and double check all changes are meaningful and have a clear purpose. For example, the main branch has this code:
But this PR duplicates it in two places and adds optional chaining. In one place, it's written correctly:
But in the other place, only the first property access is optional, which is wrong:
Also, the code below looks repetitive:
|
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
I agree that the changes are a bit difficult to grasp, but it's essentially the same logic that was removed from
You're correct about that—I refactored and removed the re-initialization of
It's not repetitive; it's a call to a reusable function made in different files. |
|
@Krishna2323 thanks for the explanation. I'm okay with it now. |
|
@neil-marcellini, could you re-review this please? |
|
@QichenZhu Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
I have not been able to get to this pull request review yet. The past two weeks have been very busy and I've been focusing on internal PR reviews. I'm going to be out of office July 4th through 16th. Working 0%. I trust that Tim will move it forward while I'm gone. Thank you 🙇 |
QichenZhu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved but another issue still happens: #62747 (review). Since it has a different cause and this PR is already quite big, I suggest making a separate PR.
| const {lastWorkspacesTabNavigatorRoute, workspacesTabState} = getWorkspaceNavigationRouteState(); | ||
| const params = workspacesTabState?.routes?.at(0)?.params as WorkspaceSplitNavigatorParamList[typeof SCREENS.WORKSPACE.INITIAL]; | ||
|
|
||
| if (!lastWorkspacesTabNavigatorRoute || lastWorkspacesTabNavigatorRoute.name !== NAVIGATORS.WORKSPACE_SPLIT_NAVIGATOR || !params?.policyID) { | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this logic need to be inside the selector? It sort of doesn't look like it does, so it's kind of a lot of logic to have run everytime the selector is ran (which is once for every policy in the collection). Can some of it be moved outside the selector?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to move the logic then we would need to set state for lastWorkspacesTabNavigatorRoute, workspacesTabState and update it when navigation is triggered, otherwise it won't work as expected.
Should I implement this?
const initialNavigationRouteState = getWorkspaceNavigationRouteState();
const [lastWorkspacesTabNavigatorRoute, setLastWorkspacesTabNavigatorRoute] = useState(initialNavigationRouteState.lastWorkspacesTabNavigatorRoute);
const [workspacesTabState, setWorkspacesTabState] = useState(initialNavigationRouteState.workspacesTabState);
const params = workspacesTabState?.routes?.at(0)?.params as WorkspaceSplitNavigatorParamList[typeof SCREENS.WORKSPACE.INITIAL];
useEffect(() => {
const newWorkspacesTabState = getWorkspaceNavigationRouteState();
const newLastRoute = newWorkspacesTabState.lastWorkspacesTabNavigatorRoute;
const newTabState = newWorkspacesTabState.workspacesTabState;
setLastWorkspacesTabNavigatorRoute(newLastRoute);
setWorkspacesTabState(newTabState);
}, [navigationState]);
const [lastViewedPolicy] = useOnyx(
ONYXKEYS.COLLECTION.POLICY,
{
canBeMissing: true,
selector: (val) => {
if (!lastWorkspacesTabNavigatorRoute || lastWorkspacesTabNavigatorRoute.name !== NAVIGATORS.WORKSPACE_SPLIT_NAVIGATOR || !params?.policyID) {
return undefined;
}
return val?.[`${ONYXKEYS.COLLECTION.POLICY}${params.policyID}`];
},
},
[navigationState],
);There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so! I like that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated 🙇🏻
|
|
||
| const navigateToWorkspacesPage = ({currentUserLogin, shouldUseNarrowLayout, policy}: Params) => { | ||
| const {lastWorkspacesTabNavigatorRoute, topmostFullScreenRoute} = getWorkspaceNavigationRouteState(); | ||
| if (!topmostFullScreenRoute) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add some comments to the code that clearly explains WHY each of these alternate navigation paths are necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comments added.
| ], | ||
| }); | ||
|
|
||
| (PolicyUtils.shouldShowPolicy as jest.Mock).mockReturnValue(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there also be a unit test for when shouldShowPolicy returns false?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added a test to handle the case when shouldShowPolicy returns false.
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
…yx selector. Signed-off-by: krishna2323 <belivethatkg@gmail.com>
tgolen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/tgolen in version: 9.1.78-0 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.1.78-4 🚀
|

Explanation of Change
Fixed Issues
$ #62569
PROPOSAL: #62569 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
android_native.mp4
Android: mWeb Chrome
android_chrome.mp4
iOS: Native
ios_native.mp4
iOS: mWeb Safari
ios_safari.mp4
MacOS: Chrome / Safari
web_chrome.mp4
MacOS: Desktop
desktop_app.mp4