-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
fix: app goes to workspace initial page when back from not found page #49226
Conversation
@shubham1206agra 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] |
@shubham1206agra Plz hold up your review. This issue is not fixed when deep linking. Will tell you once it's resolved. |
@adamgrzybowski Tagging you here for additional review. |
src/libs/Navigation/Navigation.ts
Outdated
return CommonActions.reset({ | ||
...state, | ||
routes, | ||
index: routes.length < state.routes.length ? state.index - 1 : state.index, |
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.
index: routes.length - 1
should be enough. The index for the stack points to the last element of the array.
BTW the current implementation for calculating the index would break if there is more than one screen with given name. It's not possible for the workspace initial but it is possible for other screens.
Maybe we should limit this function to filter out just the last found route with this name? It may make sense even with fixed index
implementation to limit this function.
Also maybe we don't have to call reset if we haven't removed any route?
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.
After testing, I found this function does not work properly since the Workspace_Initial
is always inside FullScreenNavigator
, so we cannot filter out the Workspace_Initial
by using state.routes?.filter((item) => item.name !== screen)
. It prevents the app from working properly when deep linking.
src/libs/Navigation/AppNavigator/createCustomFullScreenNavigator/CustomFullScreenRouter.tsx
Show resolved
Hide resolved
I left some small comments, but overall I think it's good 🚀 |
@adamgrzybowski I left a comment here. Still working to fix that. |
@adamgrzybowski Idk why the |
@dominictb I can look into this 👁️ 👁️ |
hey @dominictb what reproduction steps should trigger the |
@adamgrzybowski Navigating to an invalid workspace ID link by deep liking:
|
Okay, I found the problem. The I made it work with some refs etc but I don't think we should use hooks in the There should be a way to get this information without using hooks. |
Sorry this is quite complicated. I need time for thorough investigation. Will provide update tomorrow. |
@dominictb Can you convert this PR to draft? |
I moved |
@dominictb Bump here |
The deep linking scenario has many cases to handle. This requires very careful testing. I'll provide update today. |
@shubham1206agra I fixed both the wide screen and deep linking issues. You can retest now. |
@shubham1206agra How's it going? |
@dominictb I think the initial screen is getting popped out incorrectly. Screen.Recording.2024-11-07.at.11.21.31.AM.mov |
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2024-11-07.at.12.21.49.PM.movAndroid: mWeb ChromeScreen.Recording.2024-11-07.at.11.26.34.AM.moviOS: NativeScreen.Recording.2024-11-07.at.12.13.53.PM.moviOS: mWeb SafariScreen.Recording.2024-11-07.at.11.09.50.AM.movMacOS: Chrome / SafariScreen.Recording.2024-11-07.at.11.03.59.AM.movMacOS: DesktopScreen.Recording.2024-11-07.at.11.34.23.AM.mov |
@lakchote Please wait for @dominictb's confirmation for #49226 (comment) on main. |
@shubham1206agra @lakchote My testing indicates that the problem happens on |
@Expensify/design what do you think? |
Hmm yeah that does feel like it's not working as expected. I'm not too familiar with how the Android back button works but I would expect it to just take you back to the previous page you were on. |
✋ 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/lakchote in version: 9.0.59-0 🚀
|
@Julesssss it works correctly on staging: ScreenRecording_11-11-2024.09-18-38_1.MP4@shubham1206agra why did you create a revert PR? |
Just to test a deploy blocker. I will close it. |
🚀 Deployed to production by https://github.com/francoisl in version: 9.0.59-3 🚀
|
Details
Fixed Issues
$ #46646
PROPOSAL: #46646 (comment)
Tests
[Native only]
/settings/workspaces/1/more-features
)Verify app goes back to workspaces list page
Verify app goes back to workspaces list page
Offline tests
QA Steps
[Native only]
/settings/workspaces/1/more-features
)Verify app goes back to workspaces list page
Verify app goes back to workspaces list page
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Screen.Recording.2024-09-16.at.16.21.46.mov
Android: mWeb Chrome
Screen.Recording.2024-09-16.at.02.46.07.mov
iOS: Native
Screen.Recording.2024-09-16.at.02.40.23.mov
iOS: mWeb Safari
Screen.Recording.2024-09-16.at.02.42.38.mov
MacOS: Chrome / Safari
Screen.Recording.2024-09-16.at.02.41.18.mov
MacOS: Desktop
Screen.Recording.2024-09-16.at.03.10.07.mov