Handle network disconnection #107

deepak2431 · 2023-07-12T07:23:19Z

This PR closes #43. It handles the network error for the DOTCOM instance. It handles the server disconnection due to network issues by adding an appropriate message. Also, add a reload button to return when the user is back in the network.

Test plan

All tests have passed.

REC-20230725162020.mp4

deepak2431 · 2023-07-12T18:12:30Z

@abeatrix Can I have a review, please?

abeatrix

i left some comments inline with some minor change request, the rest looks good to me!
Will defer to tim on the design :D

abeatrix · 2023-07-13T03:10:11Z

vscode/src/chat/ChatViewProvider.ts

-            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


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 👀

Okay so when reload button is clicked, we need to get the token and endpoint to initiate the Re-LOGIN for the user. So, this change handles both reload cases and when sign-in is done manually. But I think it would be BEST to do it in a separate message protocol. LMK, what do you think?

abeatrix · 2023-07-13T03:19:12Z

vscode/src/chat/ChatViewProvider.ts

@@ -386,8 +389,18 @@ export class ChatViewProvider implements vscode.WebviewViewProvider, vscode.Disp
                if (isAbortError(err)) {
                    return
                }
+                // check first if search error is due to any network issue
+                if (isNetworkError(err)) {
+                    void this.sendErrorToWebview(


i think instead of this.sendErrorToWebview(), we should use this.transcript.addErrorAsAssistantResponse() (or both) to avoid having missing bot response item:

i think the only time we can have a boy response item is when the message is aborted manually, what do you think? 😄

It would be good to have both of them, as ENOTFOUND error as error assistant response shows up. So, this won't be much clear for the user as to why this is happening, so it would be better to include both. LMK, if you agree to this?

Yea let's see what tim thinks but what what you suggested makes sense to me :)

Sure, will wait for him to confirm.

@toolmantim What about this?

I think outputting it into the chat as a bot response is enough, we don't need an additional error at the top also. We could restyle the error bot responses to be red or make it clearer, but that can happen in a follow-up.

That sounds good to me. To mention, for now, there's an implementation of ErrAssistantResponse to show in the chat so after removing this webview error that would be displayed in case of an error.

abeatrix · 2023-07-13T03:20:40Z

vscode/webviews/Error.tsx

+        return (
+            <>
+                <div className={styles.error}>
+                    <p>{AUTH_ERRORS.NETWORK_ERROR}</p>


I like this! Maybe we can show the endpoint address to let the user know which instance is having network issue?

That would be a good add on, I will update it.

abeatrix · 2023-07-13T03:21:50Z

vscode/webviews/Error.tsx

+            <>
+                <div className={styles.error}>
+                    <p>{AUTH_ERRORS.NETWORK_ERROR}</p>
+                    <VSCodeButton className={styles.button} type="button" onClick={handleReload}>


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 :)

Yes, we can have that so it would be align to the style with all other buttons.

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.

Got it, thanks @toolmantim

deepak2431 · 2023-07-13T08:14:23Z

@abeatrix Thanks for the review! Can network errors be handled when the Sign In is done using Cody App? I tried this but couldn't get a working solution.
Also, do you mind testing it once for the enterprise test customer instance to ensure it works perfectly for that cases too?

deepak2431 · 2023-07-23T17:29:44Z

@toolmantim Can you please look at this and add your comments about the flow and the button CSS so I can make those changes?

abeatrix · 2023-07-24T17:22:07Z

Left an in-line comment. Also adding @umpox as reviewer as he's working on refactoring the chat view that could be related 😃

deepak2431 · 2023-07-24T17:32:43Z

@abeatrix I think you forgot to submit the inline comments as I can't see them. Looking forward to review from @umpox

abeatrix

See inline comment.
Will defer to @umpox for final approval as this made changes in the refactoring work he is working on.

abeatrix · 2023-07-24T17:19:36Z

vscode/src/chat/ChatViewProvider.ts

-                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


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?

Yeah, agree that would be the case here based on the implementation logic. It would be better to separate this in different messages as reload, in which we check the config values and login the user. WDYT, LMK?

I think separating this into reload is a good idea 👍

umpox

Left a couple of comments!

umpox · 2023-07-25T10:11:51Z

vscode/webviews/Error.tsx

+    NETWORK_ERROR:
+        'Due to network-related problems, Cody cannot be reached. Try again after checking it! Once your connection is restored, attempt a reload.',


Suggested change

NETWORK_ERROR:

'Due to network-related problems, Cody cannot be reached. Try again after checking it! Once your connection is restored, attempt a reload.',

NETWORK_ERROR:

'Connection failed due to a network problem. Please check your internet connection and try again.',

Small copy change to make this a bit simpler

umpox · 2023-07-25T10:12:32Z

vscode/src/chat/ChatViewProvider.ts

-                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


I think separating this into reload is a good idea 👍

deepak2431 · 2023-07-25T11:01:58Z

@umpox @abeatrix Made all the requested changes. Also, I updated the error message in the assistant response in case of network error to be clear about that. Check the updated video in the PR description for the final behaviour.

abeatrix · 2023-07-25T20:33:42Z

@deepak2431

@umpox @abeatrix Made all the requested changes. Also, I updated the error message in the assistant response in case of network error to be clear about that. Check the updated video in the PR description for the final behaviour.

can you resolve the merge conflicts before we check out the branch please? Thanks!

deepak2431 · 2023-07-26T07:31:13Z

@abeatrix Resolved the conflicts and updated the code as per the refactor of ChatView.

abeatrix

worked as expected! Thanks @deepak2431 !!

umpox

🚀

handle network error

cd990c9

deepak2431 marked this pull request as draft July 12, 2023 07:23

add changelog

055cb8c

deepak2431 marked this pull request as ready for review July 12, 2023 18:11

abeatrix requested changes Jul 13, 2023

View reviewed changes

deepak2431 closed this Jul 23, 2023

deepak2431 reopened this Jul 23, 2023

deepak2431 added 3 commits July 24, 2023 18:11

fix conflicts

6811f02

display endpoint in err message

2885645

update err message

2b9d6a8

deepak2431 requested a review from abeatrix July 24, 2023 13:22

abeatrix requested a review from umpox July 24, 2023 17:20

abeatrix reviewed Jul 24, 2023

View reviewed changes

umpox reviewed Jul 25, 2023

View reviewed changes

update message protocol

19041b3

deepak2431 requested review from umpox and abeatrix July 25, 2023 11:10

jdorfman added clients/vscode os/macOS labels Jul 25, 2023

deepak2431 added 3 commits July 26, 2023 12:22

resolve conflicts

2f67c89

update code as per refactor

3f6c412

update changelog

3016e05

abeatrix approved these changes Jul 26, 2023

View reviewed changes

resolve conflicts

12100c8

umpox approved these changes Jul 27, 2023

View reviewed changes

abeatrix merged commit 0714046 into sourcegraph:main Jul 27, 2023
6 checks passed

deepak2431 deleted the deepak/network_disconnect branch November 14, 2023 06:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle network disconnection #107

Handle network disconnection #107

deepak2431 commented Jul 12, 2023 •

edited

deepak2431 commented Jul 12, 2023

abeatrix left a comment

abeatrix Jul 13, 2023

deepak2431 Jul 13, 2023

abeatrix Jul 13, 2023

deepak2431 Jul 13, 2023

abeatrix Jul 13, 2023

deepak2431 Jul 13, 2023

deepak2431 Jul 24, 2023

toolmantim Jul 24, 2023

deepak2431 Jul 24, 2023

abeatrix Jul 13, 2023

deepak2431 Jul 13, 2023

abeatrix Jul 13, 2023

deepak2431 Jul 13, 2023

toolmantim Jul 24, 2023

deepak2431 Jul 24, 2023

deepak2431 commented Jul 13, 2023

deepak2431 commented Jul 23, 2023

abeatrix commented Jul 24, 2023

deepak2431 commented Jul 24, 2023

abeatrix left a comment

abeatrix Jul 24, 2023

deepak2431 Jul 25, 2023

umpox Jul 25, 2023

umpox left a comment

umpox Jul 25, 2023

umpox Jul 25, 2023

deepak2431 commented Jul 25, 2023

abeatrix commented Jul 25, 2023

deepak2431 commented Jul 26, 2023

abeatrix left a comment

umpox left a comment

		NETWORK_ERROR:
		'Due to network-related problems, Cody cannot be reached. Try again after checking it! Once your connection is restored, attempt a reload.',

Handle network disconnection #107

Handle network disconnection #107

Conversation

deepak2431 commented Jul 12, 2023 • edited

Test plan

deepak2431 commented Jul 12, 2023

abeatrix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deepak2431 commented Jul 13, 2023

deepak2431 commented Jul 23, 2023

abeatrix commented Jul 24, 2023

deepak2431 commented Jul 24, 2023

abeatrix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

umpox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deepak2431 commented Jul 25, 2023

abeatrix commented Jul 25, 2023

deepak2431 commented Jul 26, 2023

abeatrix left a comment

Choose a reason for hiding this comment

umpox left a comment

Choose a reason for hiding this comment

deepak2431 commented Jul 12, 2023 •

edited