-
Notifications
You must be signed in to change notification settings - Fork 291
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
onboarding: add endpoint selection UI for non-VSCode IDEs #5541
base: main
Are you sure you want to change the base?
Changes from all commits
d8bb1b3
a2261c8
8c43907
f8c69b7
e8c7fc2
f7ac932
4152ea1
fe8eb85
1463ffe
f294950
71ff1c2
7a658d8
1b654e9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -135,6 +135,7 @@ import { InitDoer } from './InitDoer' | |
import { getChatPanelTitle } from './chat-helpers' | ||
import { type HumanInput, getPriorityContext } from './context' | ||
import { DefaultPrompter, type PromptInfo } from './prompt' | ||
import {secretStorage} from "../../services/SecretStorageProvider"; | ||
|
||
export interface ChatControllerOptions { | ||
extensionUri: vscode.Uri | ||
|
@@ -438,11 +439,17 @@ export class ChatController implements vscode.Disposable, vscode.WebviewViewProv | |
} | ||
break | ||
} | ||
if (message.authKind === 'signin' && message.endpoint && message.value) { | ||
await localStorage.saveEndpointAndToken({ | ||
serverEndpoint: message.endpoint, | ||
accessToken: message.value, | ||
}) | ||
if (message.authKind === 'signin' && message.endpoint) { | ||
const serverEndpoint = message.endpoint | ||
const accessToken = message.value ? message.value : (await secretStorage.getToken(serverEndpoint)) ?? '' | ||
const tokenSource = message.value ? 'paste' : await secretStorage.getTokenSource(serverEndpoint) | ||
const validationResult = await authProvider.validateAndStoreCredentials( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The simpler control flow of handling the missing access token seems nice. |
||
{ serverEndpoint, accessToken, tokenSource }, | ||
'always-store' | ||
) | ||
if (!validationResult.authStatus.authenticated) { | ||
await showSignInMenu() | ||
} | ||
break | ||
} | ||
if (message.authKind === 'signout') { | ||
|
@@ -505,10 +512,13 @@ export class ChatController implements vscode.Disposable, vscode.WebviewViewProv | |
const sidebarViewOnly = this.extensionClient.capabilities?.webviewNativeConfig?.view === 'single' | ||
const isEditorViewType = this.webviewPanelOrView?.viewType === 'cody.editorPanel' | ||
const webviewType = isEditorViewType && !sidebarViewOnly ? 'editor' : 'sidebar' | ||
const endpoints = localStorage.getEndpointHistory() ?? [] | ||
|
||
const uiKindIsWeb = (cenv.CODY_OVERRIDE_UI_KIND ?? vscode.env.uiKind) === vscode.UIKind.Web | ||
return { | ||
uiKindIsWeb, | ||
serverEndpoint: auth.serverEndpoint, | ||
endpointHistory: [...endpoints].reverse(), | ||
experimentalNoodle: configuration.experimentalNoodle, | ||
smartApply: this.isSmartApplyEnabled(), | ||
webviewType, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,8 +12,15 @@ import { GlobeIcon, LockKeyholeIcon } from 'lucide-react' | |
import { useCallback, useState } from 'react' | ||
import { Button } from './components/shadcn/ui/button' | ||
import { Form, FormControl, FormField, FormLabel, FormMessage } from './components/shadcn/ui/form' | ||
import { | ||
Select, | ||
SelectContent, | ||
SelectGroup, | ||
SelectItem, | ||
SelectTrigger, | ||
SelectValue, | ||
} from './components/shadcn/ui/select' | ||
import { useTelemetryRecorder } from './utils/telemetry' | ||
import { useConfig } from './utils/useConfig' | ||
|
||
/** | ||
* A component that shows the available ways for the user to sign in or sign up. | ||
|
@@ -23,8 +30,9 @@ export const AuthPage: React.FunctionComponent<React.PropsWithoutRef<LoginProps> | |
uiKindIsWeb, | ||
vscodeAPI, | ||
codyIDE, | ||
endpoints, | ||
authStatus, | ||
}) => { | ||
const authStatus = useConfig().authStatus | ||
const telemetryRecorder = useTelemetryRecorder() | ||
const otherSignInClick = (): void => { | ||
vscodeAPI.postMessage({ command: 'auth', authKind: 'signin' }) | ||
|
@@ -43,7 +51,11 @@ export const AuthPage: React.FunctionComponent<React.PropsWithoutRef<LoginProps> | |
<Button onClick={otherSignInClick}>Sign In to Your Enterprise Instance</Button> | ||
</div> | ||
) : ( | ||
<ClientSignInForm authStatus={authStatus} vscodeAPI={vscodeAPI} /> | ||
<ClientSignInForm | ||
authStatus={authStatus} | ||
vscodeAPI={vscodeAPI} | ||
endpoints={endpoints} | ||
/> | ||
)} | ||
<p className="tw-mt-4 tw-mb-0 tw-text-muted-foreground"> | ||
Learn more about <a href="https://sourcegraph.com/cloud">Sourcegraph Enterprise</a>. | ||
|
@@ -101,6 +113,14 @@ export const AuthPage: React.FunctionComponent<React.PropsWithoutRef<LoginProps> | |
)} | ||
</div> | ||
</section> | ||
{endpoints?.length && ( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @taiyab FYI organic design is happening here... |
||
<section className="tw-bg-sidebar-background tw-text-sidebar-foreground tw-border tw-border-border tw-rounded-lg tw-p-6 tw-w-full tw-max-w-md"> | ||
<h2 className="tw-font-semibold tw-text-lg tw-mb-4">Account History</h2> | ||
<div className="tw-flex tw-flex-col tw-gap-6 tw-w-full"> | ||
<EndpointSelection authStatus={authStatus} endpoints={endpoints} /> | ||
</div> | ||
</section> | ||
)} | ||
<footer className="tw-text-sm tw-text-muted-foreground"> | ||
Cody is proudly built by Sourcegraph. By signing in to Cody, you agree to our{' '} | ||
<a target="_blank" rel="noopener noreferrer" href="https://about.sourcegraph.com/terms"> | ||
|
@@ -125,6 +145,8 @@ interface LoginProps { | |
uiKindIsWeb: boolean | ||
vscodeAPI: VSCodeWrapper | ||
codyIDE: CodyIDE | ||
endpoints: string[] | ||
authStatus: AuthStatus | ||
} | ||
|
||
const WebLogin: React.FunctionComponent< | ||
|
@@ -171,6 +193,7 @@ const WebLogin: React.FunctionComponent< | |
|
||
interface ClientSignInFormProps { | ||
vscodeAPI: VSCodeWrapper | ||
endpoints: string[] | ||
authStatus?: AuthStatus | ||
className?: string | ||
} | ||
|
@@ -182,9 +205,9 @@ interface ClientSignInFormProps { | |
* It validates the input and sends the authentication information to the VSCode extension | ||
* when the user clicks the "Sign In with Access Token" button. | ||
*/ | ||
const ClientSignInForm: React.FC<ClientSignInFormProps> = ({ className, authStatus }) => { | ||
const ClientSignInForm: React.FC<ClientSignInFormProps> = ({ className, authStatus, endpoints }) => { | ||
const [formData, setFormData] = useState({ | ||
endpoint: authStatus?.endpoint ?? '', | ||
endpoint: authStatus?.endpoint ?? endpoints?.[0] ?? '', | ||
accessToken: '', | ||
}) | ||
|
||
|
@@ -218,6 +241,14 @@ const ClientSignInForm: React.FC<ClientSignInFormProps> = ({ className, authStat | |
} | ||
}, [formData.accessToken, onAccessTokenSignInClick, onBrowserSignInClick]) | ||
|
||
const serverInvalid = | ||
authStatus && | ||
!authStatus.authenticated && | ||
!authStatus.pendingValidation && | ||
(authStatus.showNetworkError || authStatus.showInvalidAccessTokenError) | ||
const showNetworkError = serverInvalid && authStatus.showNetworkError | ||
const invalidToken = (serverInvalid && authStatus.showInvalidAccessTokenError) || false | ||
|
||
return ( | ||
<Form className={className} onSubmit={onSubmit}> | ||
<FormField name="endpoint"> | ||
|
@@ -237,10 +268,7 @@ const ClientSignInForm: React.FC<ClientSignInFormProps> = ({ className, authStat | |
<GlobeIcon size={16} /> Sign In with Browser | ||
</Button> | ||
|
||
<FormField | ||
name="accessToken" | ||
serverInvalid={authStatus && !authStatus.authenticated && authStatus.showNetworkError} | ||
> | ||
<FormField name="accessToken" serverInvalid={serverInvalid}> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Out of curiosity how does that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good question! It's actually part of the component from the library we are using for UI: https://www.radix-ui.com/primitives/docs/components/form#field |
||
<FormLabel title="Access Token" /> | ||
<FormControl | ||
type="password" | ||
|
@@ -251,9 +279,15 @@ const ClientSignInForm: React.FC<ClientSignInFormProps> = ({ className, authStat | |
autoComplete="current-password" | ||
required | ||
/> | ||
<FormMessage match={() => !isSourcegraphToken(formData.accessToken)}> | ||
<FormMessage | ||
match={() => !isSourcegraphToken(formData.accessToken)} | ||
forceMatch={invalidToken} | ||
> | ||
Invalid access token. | ||
</FormMessage> | ||
{showNetworkError && ( | ||
<FormMessage>Network error. Please check your connection and try again.</FormMessage> | ||
)} | ||
<FormMessage match="valueMissing">Access token is required.</FormMessage> | ||
</FormField> | ||
<Button | ||
|
@@ -267,3 +301,57 @@ const ClientSignInForm: React.FC<ClientSignInFormProps> = ({ className, authStat | |
</Form> | ||
) | ||
} | ||
|
||
export const EndpointSelection: React.FunctionComponent< | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This UI seems odd to me. It is labeled "Account History" with a dropdown... and changing the dropdown signs you in. I would expect a button to commit to an action like that, or at least a label that implies this is something "active". Another thing that's weird about reacting to change: In the one-endpoint case, which is the common case, there's no way to trigger the action because you can't "change" it. |
||
React.PropsWithoutRef<{ | ||
authStatus: AuthStatus | ||
endpoints: string[] | ||
}> | ||
> = ({ authStatus, endpoints }) => { | ||
// No endpoint history to show. | ||
if (!endpoints.length) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have you considered a design where, if there's only one endpoint, we show you that instead of a one-element dropdown? |
||
return null | ||
} | ||
|
||
const [selectedEndpoint, setSelectedEndpoint] = useState<string | undefined>(authStatus.endpoint) | ||
|
||
const onChange = useCallback( | ||
(endpoint: string) => { | ||
setSelectedEndpoint(endpoint) | ||
// The user was already authenticated with an invalid token. Let's not send another auth request. | ||
if (endpoint === authStatus?.endpoint) { | ||
return | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This also strikes me as odd... If you're in that state, why not try to help by routing them to the sign in flow? |
||
} | ||
getVSCodeAPI().postMessage({ | ||
command: 'auth', | ||
authKind: 'signin', | ||
endpoint: endpoint, | ||
}) | ||
}, | ||
[authStatus] | ||
) | ||
|
||
return ( | ||
<div className="tw-flex tw-flex-col tw-gap-6 tw-w-full"> | ||
<Select onValueChange={(v: string) => onChange(v)} value=""> | ||
<SelectTrigger className="tw-w-full"> | ||
<SelectValue className="tw-w-full" placeholder={selectedEndpoint} /> | ||
</SelectTrigger> | ||
<SelectContent position="item-aligned" className="tw-w-full tw-m-2 tw-bg-muted"> | ||
<SelectGroup className="tw-w-full" key="instances"> | ||
{endpoints?.map(endpoint => ( | ||
<SelectItem key={endpoint} value={endpoint} className="tw-w-full tw-p-3"> | ||
{endpoint} | ||
</SelectItem> | ||
))} | ||
</SelectGroup> | ||
</SelectContent> | ||
</Select> | ||
{!authStatus.authenticated && authStatus.endpoint === selectedEndpoint && ( | ||
<p className="tw-mt-2 tw-mb-0 tw-text-red-500"> | ||
Sign in failed. Try re-authenticate again with the forms above. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Grammar: Try to re-authenticate ... Also you're introducing a new word here, "authenticate", when above it is "Sign-in sign-in sign-in." So we should say "sign in again" so people know don't have uncertainty about whether re-authenticating means sign-in. Also... we have an endpoint selected and we know we need to sign in... can we just give them a button to fix the problem, instead of making them click buttons above? You know that there's a 75%+ chance they click on one of the Free/Pro sign-in buttons, because the instance sign-in button didn't work... |
||
</p> | ||
)} | ||
</div> | ||
) | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -106,7 +106,9 @@ export const CodyPanel: FunctionComponent< | |||||
/> | ||||||
)} | ||||||
{view === View.Prompts && <PromptsTab setView={setView} />} | ||||||
{view === View.Account && <AccountTab setView={setView} />} | ||||||
{view === View.Account && !clientCapabilities.isVSCode && ( | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
If you change the code to this, and then change the |
||||||
<AccountTab setView={setView} endpointHistory={config.endpointHistory || []} /> | ||||||
)} | ||||||
{view === View.Settings && <SettingsTab />} | ||||||
</TabContainer> | ||||||
<StateDebugOverlay /> | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
import * as SelectPrimitive from '@radix-ui/react-select' | ||
import { CheckIcon, ChevronDownIcon } from 'lucide-react' | ||
import React from 'react' | ||
import { cn } from '../utils' | ||
|
||
const Select = SelectPrimitive.Select | ||
const SelectContent = SelectPrimitive.Content | ||
const SelectGroup = SelectPrimitive.Group | ||
const SelectLabel = SelectPrimitive.Label | ||
const SelectValue = SelectPrimitive.Value | ||
const SelectIcon = SelectPrimitive.Icon | ||
|
||
const SelectTrigger = React.forwardRef< | ||
React.ElementRef<typeof SelectPrimitive.Trigger>, | ||
React.ComponentPropsWithoutRef<typeof SelectPrimitive.Trigger> | ||
>(({ className, children, ...props }, ref) => ( | ||
<SelectPrimitive.Trigger | ||
ref={ref} | ||
className={cn( | ||
'tw-flex tw-w-full tw-items-center tw-justify-between tw-rounded-md tw-border tw-border-border tw-bg-muted', | ||
className | ||
)} | ||
{...props} | ||
> | ||
{children} | ||
<SelectIcon asChild> | ||
<ChevronDownIcon /> | ||
</SelectIcon> | ||
</SelectPrimitive.Trigger> | ||
)) | ||
|
||
const SelectItem = React.forwardRef< | ||
React.ElementRef<typeof SelectPrimitive.Item>, | ||
React.ComponentPropsWithoutRef<typeof SelectPrimitive.Item> | ||
>(({ className, children, ...props }, ref) => ( | ||
<SelectPrimitive.Item ref={ref} className={cn('tw-w-full tw-bg-muted', className)} {...props}> | ||
<SelectPrimitive.ItemText>{children}</SelectPrimitive.ItemText> | ||
<SelectPrimitive.ItemIndicator className="tw-w-full tw-bg-muted"> | ||
<CheckIcon /> | ||
</SelectPrimitive.ItemIndicator> | ||
</SelectPrimitive.Item> | ||
)) | ||
|
||
export { Select, SelectContent, SelectItem, SelectGroup, SelectLabel, SelectTrigger, SelectValue } |
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.
Style nit, can we chain
a ?? b ?? ''
here?