refactor: onboarding after install #148
Conversation
🦋 Changeset detectedLatest commit: b3910e4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
After looking at this closer, there is a |
|
|
||
| @State() | ||
| hasOpenedInstall: boolean; | ||
| @Prop() appUrl: string; |
There was a problem hiding this comment.
I wonder if we should use the redirectUrl in place of appUrl
There was a problem hiding this comment.
Do you mean the redirectTo url set in the authOptions by the app? I wasn't sure if we could rely on that since it is optional? From comments in the wallet PR, maybe we should keep track of the specific tabId instead in case there are several tabs of the app open?
There was a problem hiding this comment.
yeah, I think keeping track of the tabId makes the most sense, but if possible we should try to implement any redirect behavior in a different PR.
There was a problem hiding this comment.
if we can scope this change to basically be "the app will update when the extension has been installed" or basically everything other than redirection, i think that would make our lives easier. things get tricky when trying to redirect, and it's a bigger project which i think requires some UX considerations
There was a problem hiding this comment.
I'm not sure then that any of this work satisfies closing the 2 linked issues? Should I just make a third issue with that scope and leave those 2 open for the new design work?
There was a problem hiding this comment.
With this scope of just reloading, I should prob keep using the url (not a specific tabId) and reload any tabs open with the app url?
There was a problem hiding this comment.
based on this issue: #144 we should not do any reloading, but instead give them two options: "Head back to App" or "close tab"
There was a problem hiding this comment.
This one is where I started and it mentions not wanting the user to have to manually refresh ...the first step was getting rid of that modal: #98 which prompted them to refresh after install. I thought the goal was to it for them.
| // Save app url in the query params to be used in the ext | ||
| // background script after install to reload the app | ||
| const urlParams = new URLSearchParams(); | ||
| urlParams.set('app', this.appUrl); |
There was a problem hiding this comment.
@kyranjamie does this need sanitation you think?
There was a problem hiding this comment.
Good spot. Yes, we should validate that this is a URL. What happens if the user enters: javascript:window.alert("lol") ?
There was a problem hiding this comment.
What is the best way to do this?
There was a problem hiding this comment.
I don't totally understand this, the user isn't entering anything here ...it is taken from window.location.href from where they launched the install modal.
There was a problem hiding this comment.
There was a problem hiding this comment.
anytime we are taking a URL from some arbitrary location we should sanitize it, anything can be hijacked
There was a problem hiding this comment.
Yeah indeed, the href could be set maliciously. We have a 1:1 later Fara, I'll run you through how one of the h1 exploit reports figured out an XSS ☠️
packages/connect/src/utils.ts
Outdated
| @@ -1,7 +1,3 @@ | |||
| export function getStacksProvider() { | |||
| return window.StacksProvider || window.BlockstackProvider; | |||
There was a problem hiding this comment.
We are no longer injecting BlockstackProvider
| return window.StacksProvider || window.BlockstackProvider; | |
| return window.StacksProvider; |
fbwoolf
changed the title
fbwoolf
force-pushed
the
refactor/onboarding-after-install
branch
from
cda7b12
to
b3910e4
Compare
fbwoolf
changed the title
Description
This PR changes the onboarding flow so that now after install the user is directed back to their app tab and it reloads the page so the extension can be detected as installed. This will be paired with changes to the stacks-web-wallet.
For details refer to issue #44 and #98
Type of Change
Does this introduce a breaking change?
No.
Are documentation updates required?
No.
Testing information
Testing will be handled by devs.
Checklist
yarn lerna run testpasses