sourcegraph icon indicating copy to clipboard operation
sourcegraph copied to clipboard

Published 20 hours ago • verified publisher icon sourcegraph

Cody: Hide feedback button on err response

Open deepak2431 opened this issue 2 years ago • 4 comments

This PR implements the code to hide the feedback button when there's an error response shown in the Chat.

Test plan

All tests have passed (unit, integration, E2E). https://www.loom.com/share/d7f4d4da793b4545bf49d242f446783b?sid=228755f8-1c17-4831-ac66-42b73d773963

deepak2431 avatar Jun 29 '23 11:06 deepak2431

@thenamankumar @toolmantim @philipp-spiess Can I have a review please?

deepak2431 avatar Jun 29 '23 11:06 deepak2431

@deepak2431 There seems to be some merge conflicts, do you mind fixing them first? 🙇

philipp-spiess avatar Jun 29 '23 13:06 philipp-spiess

@philipp-spiess Resolved the conflicts!

deepak2431 avatar Jun 29 '23 14:06 deepak2431

One thing to mention this PR will affect the Web and Cody App too I think. So, it would be good if Naman could have a look at it as the prop isTranscriptError passed in the cody-ui TranscriptItem is not optional.

deepak2431 avatar Jun 30 '23 11:06 deepak2431

Before I review the PR, why should we not have feedback buttons for erred responses? User can definitely nudge for negative feedback, and it is useful. Unless this is a huge product ask, I see this as an added complexity.

thenamankumar avatar Jul 03 '23 10:07 thenamankumar

@thenamankumar Agreed with you here that the user can nudge for negative feedback. But whenever there's an error, we are already logging that info. And the negative feedback received from the error response can cause bias as it would be different from negative feedback received from any of the valid Cody responses. That's why I thought we shouldn't have a feedback button in case of an error response. Let me know what you think!

deepak2431 avatar Jul 06 '23 08:07 deepak2431

@thenamankumar Also, I can make the props optional for now, so the Web and Cody App team can implement this whenever they want.

deepak2431 avatar Jul 06 '23 08:07 deepak2431

@deepak2431 cool, lets definitely make the prop optional. Also, I don't think it should be difficult to make this minor change for web too in the same PR. Otherwise it just creates tech-debt. Even if you do not implement it for web, please do test that it is not broken there.

thenamankumar avatar Jul 09 '23 04:07 thenamankumar

@thenamankumar Thanks. I will make the prop optional. I will try to implement it for the web and Cody app too. Let me dive into its codebase.

deepak2431 avatar Jul 09 '23 07:07 deepak2431

semi-automated message Cody's source code has moved to a new repository, https://github.com/sourcegraph/cody. If you intend to merge this change, please reopen the PR there.

In case it's helpful, here are some of the path mappings from the old repository (https://github.com/sourcegraph/sourcegraph) to the new repository (https://github.com/sourcegraph/cody):

  • client/cody/ -> vscode/
  • client/cody-shared/ -> lib/shared/
  • client/cody-ui/ -> lib/ui/

Thank you for the PR, and sorry for the inconvenience!

sqs avatar Jul 10 '23 11:07 sqs