-
Notifications
You must be signed in to change notification settings - Fork 29
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
Enable saving of Studio access token #3235
Conversation
import { EmptyState } from '../../shared/components/emptyState/EmptyState' | ||
import { Button } from '../../shared/components/button/Button' | ||
|
||
export const Studio: React.FC = () => { |
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.
Function Studio
has 39 lines of code (exceeds 30 allowed). Consider refactoring.
I think we'll want to have the webview regardless so that we have something with plenty of space/scope to direct users when they click "Share to Studio" and they aren't authed/have a token. |
65c4dfb
to
ed5dd88
Compare
import { Button } from '../../shared/components/button/Button' | ||
import { sendMessage } from '../../shared/vscode' | ||
|
||
export const Studio: React.FC = () => { |
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.
Function Studio
has 48 lines of code (exceeds 30 allowed). Consider refactoring.
Posted a demo back in #3077 |
extension/package.json
Outdated
@@ -590,6 +595,15 @@ | |||
"description": "%config.pythonPath.description%", | |||
"type": "string", | |||
"default": null | |||
}, | |||
"dvc.studioAccessToken": { |
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.
[I] Need to change to use the SecretStorage
API and not save the token in the open...
@@ -48,7 +48,7 @@ export abstract class BaseRepository< | |||
this.dvcRoot, | |||
this.webviewIcon, | |||
viewColumn, | |||
this.viewKey === ViewKey.SETUP | |||
[ViewKey.CONNECT, ViewKey.SETUP].includes(this.viewKey) |
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.
[I] Try and remove the bypass dvcRoot logic (maybe split the class)
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.
needs a follow up, I've taken a note.
{ | ||
"id": "dvc.views.studio", | ||
"name": "Studio", | ||
"when": "!dvc.studio.connected" |
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.
[F] We only show the tree view when there is no Studio token available in the secrets store. It can still be hidden by the user. We also give them the option to remove the token and save another one.
extension/src/connect/token.ts
Outdated
if (!text) { | ||
return false | ||
} | ||
return text.startsWith('isat_') && text.length === 54 |
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.
[F] This was the basic format that I gathered from each token. With the rebrand, this may change but lets see.
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.
My token was actually 53
characters long 😅
Screen.Recording.2023-02-15.at.8.19.12.AM.mov
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.
I'll relax the requirement
window.addEventListener('message', handler) | ||
return () => window.removeEventListener('message', handler) | ||
handler && window.addEventListener('message', handler) | ||
return () => handler && window.removeEventListener('message', handler) |
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.
[Q] Any objections to this? I don't need to send this webview any data right now, it is either on or off. This will change if/when we expand the webview to connect to other products.
export class Connect extends BaseRepository<undefined> { | ||
public readonly viewKey = ViewKey.CONNECT | ||
|
||
private readonly secrets: SecretStorage |
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.
[F] This seems like a better API to use instead of saving the token directly into the User's config.
Need to add at least "share to Studio" as a command before we can ship this. Added |
sendMessage({ type: MessageFromWebviewType.OPEN_STUDIO_PROFILE }) | ||
|
||
const saveStudioToken = () => | ||
sendMessage({ type: MessageFromWebviewType.SAVE_STUDIO_TOKEN }) |
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.
Since none of these functions rely on props on state, I'd move them outside of the component as these get recreated on each re-render. This should fix the 2 codeclimate issues as well.
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.
🙏🏻
"view": "dvc.views.studio", | ||
"contents": "[$(plug) Connect](command:dvc.showConnect)", | ||
"when": "!dvc.studio.connected" | ||
}, |
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.
[I] Double check on initial view placement.
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.
Great work!
extension/src/connect/token.ts
Outdated
if (!text) { | ||
return false | ||
} | ||
return text.startsWith('isat_') && text.length === 54 |
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.
My token was actually 53
characters long 😅
Screen.Recording.2023-02-15.at.8.19.12.AM.mov
return | ||
} | ||
|
||
return this.storeSecret(STUDIO_ACCESS_TOKEN_KEY, token) |
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.
Should we add some kind of message after the token is saved? Feels a little weird to me that I saved my token then everything related to Studio just disappears from my screen 🤔
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.
Yes I will add a toast/modal
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.
This is what the flow looks like now:
Screen.Recording.2023-02-16.at.4.33.03.pm.mov
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.
Let me know what you think. We can update in a follow up.
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.
Looks good! Makes things more clear on the connection and what you can do now that you're connected!
@@ -6,3 +6,9 @@ export const warnOfConsequences = ( | |||
...items: Response[] | |||
): Thenable<string | undefined> => | |||
window.showWarningMessage(text, { modal: true }, ...items) | |||
|
|||
export const showInformation = ( |
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.
Similar blocks of code found in 2 locations. Consider refactoring.
import { EmptyState } from '../../shared/components/emptyState/EmptyState' | ||
import { Button } from '../../shared/components/button/Button' | ||
|
||
export const Studio: React.FC = () => { |
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.
Function Studio
has 41 lines of code (exceeds 30 allowed). Consider refactoring.
Code Climate has analyzed commit 02a0fc3 and detected 3 issues on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 94.3% (85% is the threshold). This pull request will bring the total coverage in the repository to 95.9% (0.0% change). View more on Code Climate. |
1/3
main
<- this <- #3289 <- #32941/2
main
<- this <- #3277This PR adds a new connect button to our sidebar panel and a screen that walks the user through generating/saving a Studio access token in the extension. It also adds add/remove commands for the access token which can be used to bypass the webview. Once the user has a token they will be able to share experiments with Studio from the experiments table.
Demo
Screen.Recording.2023-02-14.at.1.28.14.pm.mov
See comments inline.
All prototype work is shown below the line.
(Very rough) Prototype for connecting the extension to Studio. This is something that we can implement quickly before investing the time to roll out the OAuth flow.
In order to progress + get down some ideas I have started prototyping for #3077. As stated here. My current plan is to get the user to save a token into their local VS Code config under the
dvc.studioToken
key. We will then use that token to share experiments to Studio from various places in the UI.Visibility:
In order to let users know that they can/should connect to Studio we have to let them know that this is possible. I think we should add a welcome view to the sidebar whenever the
dvc.studioToken
isnull
:User assistance:
This webview can either get expanded or dropped in the future depending on the outcome of the discussions in #3077. For now, it should provide a way for the user to jump through the required hoops. Button actions are described under the screenshot
Sign In
: Opensstudio.iterative.ai
in the browser.Get token
: Prompts user for theirusername
and redirects them tohttps://studio.iterative.ai/user/{username}/profile
Save
: Opens quick input where the user can paste the token.Alternative version (that I have dismissed):