-
Notifications
You must be signed in to change notification settings - Fork 213
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
Handle network disconnection #107
Changes from 5 commits
cd990c9
055cb8c
6811f02
2885645
2b9d6a8
19041b3
2f67c89
3f6c412
3016e05
12100c8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -278,9 +278,12 @@ export class ChatViewProvider implements vscode.WebviewViewProvider, vscode.Disp | |
// cody.auth.signin or cody.auth.signout | ||
await vscode.commands.executeCommand(`cody.auth.${message.type}`) | ||
break | ||
case 'settings': | ||
await this.authProvider.auth(message.serverEndpoint, message.accessToken, this.config.customHeaders) | ||
case 'settings': { | ||
const endpoint = message.serverEndpoint.length ? message.serverEndpoint : this.config.serverEndpoint | ||
const token = message.accessToken.length ? message.accessToken : this.config.accessToken | ||
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. What if the user manually input a new endpoint but didn't provide an access toke? That means we will try to log the user in with the access token from this.config that is meant to use with the endpoint in this.config? 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. Yeah, agree that would be the case here based on the implementation logic. It would be better to separate this in different messages as 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. I think separating this into |
||
await this.authProvider.auth(endpoint, token, this.config.customHeaders) | ||
break | ||
} | ||
case 'insert': | ||
await this.insertAtCursor(message.text) | ||
break | ||
|
@@ -917,6 +920,7 @@ export class ChatViewProvider implements vscode.WebviewViewProvider, vscode.Disp | |
return | ||
} | ||
const searchErrors = this.codebaseContext.getEmbeddingSearchErrors() | ||
|
||
// Display error message as assistant response for users with indexed codebase but getting search errors | ||
if (this.codebaseContext.checkEmbeddingsConnection() && searchErrors) { | ||
this.transcript.addErrorAsAssistantResponse(searchErrors) | ||
|
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
@@ -1,5 +1,9 @@ | ||||||||||
import { VSCodeButton } from '@vscode/webview-ui-toolkit/react' | ||||||||||
|
||||||||||
import { AuthStatus, isLocalApp } from '../src/chat/protocol' | ||||||||||
|
||||||||||
import { getVSCodeAPI } from './utils/VSCodeApi' | ||||||||||
|
||||||||||
import styles from './Login.module.css' | ||||||||||
|
||||||||||
const AUTH_ERRORS = { | ||||||||||
|
@@ -10,6 +14,8 @@ const AUTH_ERRORS = { | |||||||||
EMAIL_NOT_VERIFIED: 'Email not verified. Please add a verified email to your Sourcegraph.com account.', | ||||||||||
APP_NOT_RUNNING: 'Cody App is not running. Please open the Cody App to continue.', | ||||||||||
INVALID_URL: 'Connection failed due to invalid URL. Please enter a valid Sourcegraph instance URL.', | ||||||||||
NETWORK_ERROR: | ||||||||||
'Due to network-related problems, Cody cannot be reached. Try again after checking it! Once your connection is restored, attempt a reload.', | ||||||||||
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
Small copy change to make this a bit simpler |
||||||||||
} | ||||||||||
export const ErrorContainer: React.FunctionComponent<{ | ||||||||||
authStatus: AuthStatus | ||||||||||
|
@@ -26,11 +32,12 @@ export const ErrorContainer: React.FunctionComponent<{ | |||||||||
requiresVerifiedEmail, | ||||||||||
hasVerifiedEmail, | ||||||||||
siteVersion, | ||||||||||
showNetworkError, | ||||||||||
} = authStatus | ||||||||||
// Version is compatible if this is an insider build or version is 5.1.0 or above | ||||||||||
// Right now we assumes all insider builds have Cody enabled | ||||||||||
// NOTE: Insider build includes App builds but we should seperate them in the future | ||||||||||
if (!authenticated && !showInvalidAccessTokenError) { | ||||||||||
if (!authenticated && !showInvalidAccessTokenError && !showNetworkError) { | ||||||||||
return null | ||||||||||
} | ||||||||||
// Errors for app are handled in the ConnectApp component | ||||||||||
|
@@ -45,6 +52,26 @@ export const ErrorContainer: React.FunctionComponent<{ | |||||||||
const isVersionCompatible = isInsiderBuild || siteVersion >= '5.1.0' | ||||||||||
const isVersionBeforeCody = !isVersionCompatible && siteVersion < '5.0.0' | ||||||||||
const prefix = `Failed: ${isApp.isRunning ? 'Cody App' : endpoint}` | ||||||||||
|
||||||||||
const handleReload = (): void => { | ||||||||||
getVSCodeAPI().postMessage({ command: 'settings', serverEndpoint: '', accessToken: '' }) | ||||||||||
} | ||||||||||
|
||||||||||
// When there's a network error | ||||||||||
if (showNetworkError) { | ||||||||||
return ( | ||||||||||
<> | ||||||||||
<div className={styles.error}> | ||||||||||
<p>Current endpoint: {endpoint}</p> | ||||||||||
<p>{AUTH_ERRORS.NETWORK_ERROR}</p> | ||||||||||
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. I like this! Maybe we can show the endpoint address to let the user know which instance is having network issue? 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. That would be a good add on, I will update it. |
||||||||||
<VSCodeButton className={styles.button} type="button" onClick={handleReload}> | ||||||||||
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. maybe update the button styles to have full width like the rest of the buttons in the login page? Will defer to @toolmantim who will be back next week from his PTO :) 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. Yes, we can have that so it would be align to the style with all other buttons. 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. I'd love us to move away from the-full width buttons… but it's good to be consistent. But given it's an error state, I'm okay with just leaving this as a normal button w/ inherent width. 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. Got it, thanks @toolmantim |
||||||||||
Reload | ||||||||||
</VSCodeButton> | ||||||||||
</div> | ||||||||||
</> | ||||||||||
) | ||||||||||
} | ||||||||||
|
||||||||||
// When doesn't have a valid token | ||||||||||
if (showInvalidAccessTokenError) { | ||||||||||
return ( | ||||||||||
|
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.
Can you tell me more about this change?
This is called when a user input the endpoint and access token manually, so it doesn't make sense to me to replace them with the ones from this.config if those are not available 👀
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.
Okay so when
reload
button is clicked, we need to get the token and endpoint to initiate theRe-LOGIN
for the user. So, this change handles bothreload
cases and whensign-in
is done manually. But I think it would be BEST to do it in a separate message protocol. LMK, what do you think?