Fix workspace not created after deeplinking to newDot #54992
Conversation
|
🚧 @mountiny has triggered a test build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
Not needed anymore |
|
Sorry mixed the PRs |
537eb44 to
668a269
Compare
|
🚧 @mountiny has triggered a test build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
|
🚧 @mountiny has triggered a test build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
|
🚧 @mountiny has triggered a test build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
|
🚧 @mountiny has triggered a test build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
|
🚧 @mountiny has triggered a test build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
|
🚧 @mountiny has triggered a test build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
|
🚧 @mountiny has triggered a test build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
7fecc50 to
fcaf593
Compare
|
@hungvu193 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] |
There was a problem hiding this comment.
@hungvu193 has tested and confirmed they cannot reproduce the original issue anymore and also that the desktop deplink works. This is however quite lower level change where we could expect some edge cases to break. However, since the original issue is quite impactful to our real customers, I would like to move this ahead to staging and let QA do regression testing.
If they find some regressions, we can revert this @nkdengineer can you please prepare revert of this PR as draft after its merged so we are ready to fix any issues that might come up? thanks!
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2025-01-14.at.19.56.53.mp4MacOS: Desktop |
@mountiny Sure. |
|
@nkdengineer Notice this issue, where the onboarding modal was shown but the current report wasn't Concierge. the.second.mp4Also if I opened many tabs at the same times while reproducing the issue, sometimes one of the tab will be crashed. I'm checking staging env if I got similar issues. |
|
✋ 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/mountiny in version: 9.0.85-0 🚀
|
3 similar comments
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.0.85-0 🚀
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.0.85-0 🚀
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.0.85-0 🚀
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.0.85-0 🚀
|
|
@izarutskaya This is a backend bug and it's fixed now. |
|
@nkdengineer if Tom is still able to reproduce, i feel like we should revert this pr as there is no good reason to keep it |
|
@nkdengineer @mountiny On Desktop we don't see 500 error on step 7, but we can see infinite spinner. Otherwise the issue is the same Pr54992.mp4Prod: Prod.test.edit.mp4 |
| window.addEventListener('beforeunload', () => { | ||
| // When we open route in desktop, beforeunload is fired unexpectedly here. | ||
| // So we should return early in this case to prevent cleaning the clientID | ||
| if (isOpeningRouteInDesktop()) { | ||
| resetIsOpeningRouteInDesktop(); | ||
| return; | ||
| } | ||
| cleanUpClientId(); | ||
| }); |
There was a problem hiding this comment.
The addEventListener is adding a new function everytime, and the removeEventListener won't remove the function given that cleanUpClientId doesn't exists. Probably that's why you need an extra check.
The Add/RemoveEventListener must receive the same function instance.
There was a problem hiding this comment.
@gedu So we can move this into cleanUpClientId function, right?
if (isOpeningRouteInDesktop()) {
resetIsOpeningRouteInDesktop();
return;
}
There was a problem hiding this comment.
It depends, if doing
window.addEventListener('beforeunload', cleanUpClientId);
desktop doesn't fire beforeunload, probably is not needed, but if just to really make sure, maybe yes, can be inside of cleanUpClientId
|
@nkdengineer can you check the video and the comment from Edu? |
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.0.85-4 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.0.85-4 🚀
|
Explanation of Change
Fixed Issues
$ #54849
PROPOSAL: #54849
Tests
Unable to flush. Client is not the leader.to the search log and clear all logsLogOutAPI is called and it fails, refresh the page)Offline tests
None
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Unable to flush. Client is not the leader.to the search log and clear all logsLogOutAPI is called and it fails, refresh the page)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: mWeb Chrome
Screen.Recording.2025-01-14.at.18.33.32.mov
iOS: Native
iOS: mWeb Safari
Screen.Recording.2025-01-14.at.18.35.45.mov
MacOS: Chrome / Safari
Screen.Recording.2025-01-14.at.18.27.12.mp4
MacOS: Desktop
Screen.Recording.2025-01-14.at.18.41.49.mov