-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Workspace - On completing transfer ownership flow, tapping back button redirects to blank page #57935
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
Workspace - On completing transfer ownership flow, tapping back button redirects to blank page #57935
Conversation
@jayeshmangwani 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] |
Since the transfer owner does not appear in android and ios native, so I cannot record video in these two platform |
Yeah, I'm also unable to test on native. I'll see if there's anything we can do to make it work. |
I’m seeing a bug on mWeb Safari—let me know if you think we can do anything to solve it. Steps:
bug-swipe.mov |
@jayeshmangwani For the bug mWeb Safari, i'm investigating but haven't found solution. Will get back to you asap |
Thanks for the update! Let me know if you need any help with anything. |
@jayeshmangwani I tested the behavior with the left swipe action and noticed that the same issue occurs in other flows as well. This suggests a global issue with the sync between the screen stack and the URL history. In the video below, after creating a new task, swiping left briefly reveals the previous report screen with the composer menu popover appearing for a moment: Screen.Recording.2025-03-11.at.15.39.59.movIn the below video, after creating a new workspace, swiping left briefly reveals the new workspace form: Screen.Recording.2025-03-11.at.15.46.15.movBased on the testing results, I believe we should create a separate issue to address this general bug, as it is an existing issue that occurs in multiple flows. |
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb Chromemweb-chrome.moviOS: NativeiOS: mWeb Safarimweb-safari.movMacOS: Chrome / Safariweb.movMacOS: Desktopdesktop.mov |
Changes look good, but I'm unable to test them on native devices since the Transfer owner button isn’t visible when a fundList data is empty. I tried adding a fundList using the Onyx set, but the next page keeps crashing, preventing me from testing. However, it works well on other platforms. |
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.
LGTM 🚀
@danieldoglas I wasn't able to test on native devices yet. Please check this comment. |
@jayeshmangwani I think that the transition is not super smooth. It looks like it goes forward and then it does a back animation to show the success page. Is it possible to fix that here? |
Hey @thelullabyy, could you please check this comment? When we show the success page from the Transfer Owner flow, the back navigation appears briefly before the success animation. Can we fix this in the PR? |
@danieldoglas @jayeshmangwani Could you clarify this bug a bit more? It would help to describe the test steps and point out exactly where the issue happens so I can reproduce and debug it more effectively. By the way, is the bug you’re referring to at the 9th second of the video below, where the screen with the button (without text) briefly appears? Screen.Recording.2025-03-17.at.16.53.45.movHere’s a screenshot of that moment (9th second): ![]() |
Yes, that's exactly what I was referring to. It looks like the back navigation is happening before the success page is displayed. |
@jayeshmangwani Root cause here is we use navigate in useEffect and while the navigate command has not been executed, the WorkspaceOwnerChangeWrapperPage screen is still displayed. Currently the main is also having the same problem so I think it is not within the scope of this issue and should create a new issue to handle this. |
Ok, I agree this might be a separate scope. |
Should we go ahead with the merge for @danieldoglas ? |
✋ 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/danieldoglas in version: 9.1.16-0 🚀
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.1.16-4 🚀
|
Explanation of Change
Fixed Issues
$#57245
PROPOSAL:#57245 (comment)
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)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
Android: mWeb Chrome
android_chorme.mp4
iOS: Native
iOS: mWeb Safari
ios_safari.mp4
MacOS: Chrome / Safari
chorme.mov
MacOS: Desktop
desktop.mov