Skip to content
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

Send error data when error occurs to display user error message #17

Conversation

Deborah-Digges
Copy link
Contributor

@Deborah-Digges Deborah-Digges commented Oct 10, 2024

Description

  • An empty 500 without response text doesn't seem to be handled correctly by copilot chat
  • Sending back error text triggers the error state and shows the warning error banner ⚠️

After

Screenshot 2024-10-10 at 4 13 56 PM

Before

Screenshot 2024-10-10 at 4 36 26 PM

@Deborah-Digges Deborah-Digges marked this pull request as ready for review October 15, 2024 18:39
Copy link

@cheshire137 cheshire137 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if you could write a test that exercises this and would've failed without your change. 🤔

@@ -225,6 +225,7 @@ const server = createServer(async (request, response) => {
} catch (err) {
console.error(err);
response.statusCode = 500
response.write("data: Something went wrong\n\n")
Copy link

@cheshire137 cheshire137 Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be curious to know if the error banner shows up in chat because this data: packet isn't a JSON string like something expected, and/or because it doesn't end with data: [DONE]. Not that you should necessarily write something different here, just thinking about whether your "Something went wrong" is being used or if you could've put any non-JSON value after data: and gotten the same result. 🤷🏻‍♀️

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the actual string I'm using here is being used, but it seems like just the presence of error data seems to trigger the error state! I need to find the corresponding code in copilot chat to verify for sure though 👀

This repo doesn't have tests yet, so I'm thinking of creating an issue and following up on a separate PR to create test scaffolding

@Deborah-Digges Deborah-Digges merged commit 4067c19 into copilot-extensions:main Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants