-
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 - [CRITICAL] Multiple workspaces get created when someone either refreshes the page, or drops off and returns later. #52899
Fix - [CRITICAL] Multiple workspaces get created when someone either refreshes the page, or drops off and returns later. #52899
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] |
Working on uploading screenshots ... |
Upload finished |
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb Chromemweb-chrome.moviOS: NativeiOS.moviOS: mWeb Safarimweb-safari.movMacOS: Chrome / Safariweb.movMacOS: Desktopdesktop.mov |
@FitseTLT I'm not sure why, but on Android, a workspace is created twice if it comes from a closed app on onboarding flow. Android.mov |
I don't think so @jayeshmangwani Maybe you were on a wrong branch. I tried to exactly do what is on your video but couldn't reproduce 2024-11-21.18-09-47.mp4 |
No, thats not the case, I am on right branch |
I am checking on real device, let's see if that my emulator thing |
For me, it's still creating two workspaces |
@mountiny Can we please generate build here so that we can test it on an Android device?" |
@FitseTLT Do you have any idea, How can we open the OD web link in the ND staging version of the app? We are testing locally using |
@jayeshmangwani Sorry but I am totally not understanding what you meant. |
Sorry about that! Let me simplify it: how have you tested this PR on Android? |
|
Sorry, I didn’t quite get you. |
🚧 @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! 🧪🧪 |
@jayeshmangwani here is the build |
Thanks, I’m checking it now |
@mountiny , I am trying to test the transition link from OldDot to NewDot on a native Android device for the signup flow when we select the VSB option on OD, but for me, it is always opening in mobile web. Is there any way you are aware of to test on a native Android device? Otherwise, this issue cannot be tested in a real scenario. |
I am not sure how to test the short-lived token in your dev setup, sorry 😬 its probably not possible as the deeplinks will be picked up in the actual app, not your dev apk |
I think we can proceed with this PR and merge it, as testing on both the production web and mobile app shows that we are not opening the transition deep link on mobile (when creating a new account and selecting the VSB option). |
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 for the changes. I think we should still improve this logic eventually, if someone leaves the page mid onboarding without completing and comes back to it in another browser or client, they will get another workspace created afaik
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
If the user skips the onboarding midway, we currently do not display the onboarding modal on another browser. Instead, they are directly taken to the sidebar screen. |
We only show the uncompleted onboarding flow if the user is on the same browser |
If they do not complete the flow though they would still have the onboarding nvp set as not completed so we should show the flow, right? |
Technically, we should, yes. However, what I'm trying to highlight is that currently:
In the above two scenarios, multiple workspaces will not be created. If we need a discussion on the existing flow—specifically why a user cannot see the onboarding flow on another device if flow hasn't been completed on the first device, and why the user gets logged out on Device A where the onboarding flow was initiated, we can open a Slack discussion to get more 👀 |
This certainly feels like a bug, eny time the onboarding nvp says hasCompletedGuidedFlow: false it should show the onboarding modal |
🚀 Deployed to staging by https://github.com/mountiny in version: 9.0.66-0 🚀
|
cc @FitseTLT @jayeshmangwani this is failing in desktop and in iOS in QA testing, can you please have a look? |
🚀 Deployed to production by https://github.com/mountiny in version: 9.0.66-8 🚀
|
Details
Fixed Issues
$ #52894
PROPOSAL: #52894 (comment)
Tests
Offline tests
Same as above
QA Steps
Same as above
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label 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
a.mp4
Android: mWeb Chrome
aw.mp4
iOS: Native
i.mp4
iOS: mWeb Safari
iw.mp4
MacOS: Chrome / Safari
w.mp4
MacOS: Desktop
d.mp4