Cody: Add intent hover text #52029

deepak2431 · 2023-05-16T18:40:23Z

This PR closes #51633. It added hover text to different intent detection files.

Test plan

Before: Just plain border for the hallucinated and non-hallucinated files.

After: A nice border, with the title "Hallucination detected: file does not exist" for hallucination files and "No hallucination detected: file exists" for non-hallucinated files.

REC-20230604104616.mp4

philipp-spiess

Thanks for the contribution @deepak2431!

There are some internal discussions about the usefulness of the hallucination detection the way we have it right now but that aside, I think it would be helpful to add some form of a label in the meantime so we actually explain what this means to the user.

I noticed that it looks a bit janky to add the inline text in your video. Have you tried using position: absolute and make it look more like a tooltip instead?

You also seem to be hardcoding some colors there. Since we have to support various VS Code color themes, it would be great if you can look up VS Code color theme variable names instead. There's a cool extension I use for this: https://marketplace.visualstudio.com/items?itemName=GitHubOCTO.spectrum

Another concern I have is accessibility but I doubt we'll be able to fix this without changing the markup 😢 and I really don't want to make the markup more complex because it already can leak in some scenarios

deepak2431 · 2023-05-17T15:31:39Z

Thanks for the review again @philipp-spiess 😀. Haven’t tried with the absolute positioning but yeah let me try that and make it look much better more like a tooltip as you mentioned.

I will update the color scheme, thanks for letting me know that we need to use vs code theme with the color as I used the one declared in the Cody-UI.

Regarding the accessibility if you can let me know about some pointers to consider, I can look into if it can be possible using CSS styles.

Again, A big thanks for looking into all of my PR’s and helping to get it reviewed!

philipp-spiess · 2023-05-17T16:51:26Z

Regarding the accessibility if you can let me know about some pointers to consider, I can look into if it can be possible using CSS styles.

The issue is :hover can't be triggered for a screen reader users, so this tooltip will not be accessible to screenreader users at all.

Normally in a website you'd want to make sure you can tab to the item and have an aria-label or so to have the screenreader explain this. We'll need to change the markup for this though which is something I don't want us to do right now without fixing the bug where the markup is not rendered in inline blocks at all 🙃

Consider the following result from Cody right:

Here is some inline code: `my code path/to/a/file.md is here`

Right now this will trigger the hallucination detection logic that will replace the content before the markdown is parsed to something like this:

Here is some inline code: `my code <span>path/to/a/file.md<span> is here`

This, however will result in the <span> to be encoded since it is inside a code block so it will render in the initial response.

deepak2431 · 2023-05-17T19:13:55Z

@philipp-spiess That makes sense. I was thinking of accessibility you mentioned previously in terms of the view of hover text, color etc. But yeah this hover can cause the issue in some devices.

I have seen the story of your markdown when you was working with the HTMl entities bug. I hope down the line we would definitely be able to handle this too with a better markdown 😊

deepak2431 · 2023-05-18T18:06:29Z

REC-20230518231904.mp4

@philipp-spiess Updated the CSS. It looks much better now, I think.

philipp-spiess · 2023-05-19T09:29:39Z

@deepak2431 Okay this is pretty slick indeed! 😮

Only thing I am wondering now is if we should include a bit more context in the text. Paging the rest of the cody reviewers to think about this too :)

philipp-spiess · 2023-05-19T09:31:00Z

client/cody/webviews/Chat.module.css

+:global(.token-not-hallucinated):hover:after {
+    content: "This file exists";
+}
+
+:global(.token-hallucinated):hover:after {
+    content: "This file does not exist";
 }


I wonder if we should add more context here. Is "hallucination detection" a customer facing name we have for this feature? It's already really unclear what this is doing and just saying "file exists" is still confusing. cc @mrnugget

@deepak2431 Okay this is pretty slick indeed! 😮

Only thing I am wondering now is if we should include a bit more context in the text. Paging the rest of the cody reviewers to think about this too :)

Thanks, @philipp-spiess . Sure, will wait for the comment from @mrnugget on what he thinks should be the updated content text.

toolmantim · 2023-05-30T07:24:22Z

Thanks so much for the PR @deepak2431! 🙏🏼

@philipp-spiess @mrnugget I've checked this out locally, and have a suggested design:

Screen.Recording.2023-05-30.at.5.08.05.pm.mov

Some notes on what's included there:

Uses the title attribute
On files that that are hallucinations, add title="Hallucination Detected: file does not exist"
On files that aren't hallucinations, link them with a title="Open File" — the existence of link is enough to show they've been verified

Colors used:

Hallucinated files: --vscode-textPreformat-foreground
Verified file links: --vscode-textLink-foreground

deepak2431 · 2023-05-30T07:32:55Z

@toolmantim Thanks for looking into this; the updated design looks awesome! Let me make the changes.

…o deepak/cody-intent-highlight

deepak2431 · 2023-05-30T13:48:34Z

@toolmantim For the hallucinated files, the colour you mentioned is white, but in the new UI video looks yellow. So, can I use this colour for the hallucinated files: --vscode-problemsWarningIcon-foreground

…o deepak/cody-intent-highlight

deepak2431 · 2023-05-30T17:22:22Z

@toolmantim Made all the suggested changes. Here's how the updated version looks:

REC-20230530224537.mp4

Edit: Tests are fixed!!!

…o deepak/cody-intent-highlight

deepak2431 · 2023-06-02T05:40:48Z

@toolmantim It's all good for final review now.

toolmantim

Thanks @deepak2431! I tested locally, and left some fixes in suggestions.

It won't make sense to use the word "Open file" and color them as links until they're actually linked properly (e.g. <a href="vscode://file/(path)">(token)</a>). But that seems more involved, and we can get this PR out now to at least improve the styles and titles.

client/cody/webviews/Chat.module.css

toolmantim · 2023-06-04T00:03:46Z

client/cody-shared/src/hallucinations-detector/index.ts

@@ -88,7 +88,10 @@ function highlightLine(line: string, tokens: HighlightedToken[]): string {

 function getHighlightedTokenHTML(token: HighlightedToken): string {
    const isHallucinatedClassName = token.isHallucinated ? 'hallucinated' : 'not-hallucinated'
-    return ` <span class="token-${token.type} token-${isHallucinatedClassName}">${token.outerValue.trim()}</span> `
+    const title = token.isHallucinated ? 'Hallucination detected: file does not exist' : 'Open file'


Suggested change

const title = token.isHallucinated ? 'Hallucination detected: file does not exist' : 'Open file'

const title = token.isHallucinated ? 'Hallucination detected: file does not exist' : 'No hallucination detected: file exists'

client/cody-shared/src/hallucinations-detector/index.test.ts

toolmantim · 2023-06-04T00:18:41Z

If you could update the PR description to have some before and after screenshots too, that would be great for people understanding the changeset. Thank you!

…o deepak/cody-intent-highlight

deepak2431 · 2023-06-04T05:26:24Z

@toolmantim Thanks for the review. I have made all the changes as per your review. Also, I added the before and after screenshots in the PR description.

deepak2431 · 2023-06-04T05:29:36Z

@toolmantim Also, that makes sense to me about the href to non-hallucinated files you mentioned. Also, I feel that the current implementation for detecting hallucinated/non-hallucinated files needs improvement as it identifies the files that also exist as Hallucinated. But yeah, this would be part of other PR.

toolmantim · 2023-06-04T06:08:17Z

Thanks so much 👏🏼 I'll review this for you shortly!

it identifies the files that also exist as Hallucinated

Is there an issue filed for this? Would love some repro steps if possible!

deepak2431 · 2023-06-04T06:59:43Z

Thanks so much 👏🏼 I'll review this for you shortly!

it identifies the files that also exist as Hallucinated

Is there an issue filed for this? Would love some repro steps if possible!

Sure, Take your time!

I don't think that there's any issue created for this.
See this when I have used this prompt in this repo

But the files mentioned it is present here.

Shall I open an issue for this?

toolmantim · 2023-06-04T21:33:22Z

Hrmm, that's not good. If you could open an issue that would be great. Thanks!

deepak2431 · 2023-06-05T05:21:06Z

Hrmm, that's not good. If you could open an issue that would be great. Thanks!

Sure, will open an issue for it.

deepak2431 · 2023-06-08T05:43:36Z

@toolmantim Resolved the conflicts of this PR, can we get this merged if it's all good?

toolmantim

Now that main has updated to actually link files, we need to make one final switch.

I just checked this out locally, merged main, and made the suggested change… and it looks good. Once that's in, it's good to go! 🙌🏼

toolmantim · 2023-06-08T12:50:21Z

client/cody/webviews/Chat.module.css

 }

 :global(.token-not-hallucinated) {
-    border-bottom: 2px solid var(--vscode-testing-iconPassed);
-    word-break: break-all;
+    text-decoration: 1px dashed underline var(--vscode-testing-iconPassed);


Suggested change

text-decoration: 1px dashed underline var(--vscode-testing-iconPassed);

text-decoration: underline

Now that main has updated to actually link files, we need to make one final switch.

I just checked this out locally, merged main, and made the suggested change… and it looks good. Once that's in, it's good to go! 🙌🏼

Thanks for having a look @toolmantim. I will make those changes soon, and update you!

deepak2431 · 2023-06-08T15:27:27Z

@toolmantim Updated the CSS as per your suggestions.

deepak2431 · 2023-06-14T12:16:21Z

@toolmantim All good in this PR? As I can see the build falling again here!

Addressed in the subsequent changes

This PR closes #51633. It added hover text to different intent detection files. ## Test plan Before: Just plain border for the hallucinated and non-hallucinated files. ![iScreen Shoter - Code - 230604104911](https://github.com/sourcegraph/sourcegraph/assets/44617923/0a5f6d51-dc41-492c-bf8f-228726bf068b) After: A nice border, with the title "Hallucination detected: file does not exist" for hallucination files and "No hallucination detected: file exists" for non-hallucinated files. https://github.com/sourcegraph/sourcegraph/assets/44617923/6e415e99-4d1a-48be-ae32-d683fd146b85 --------- Co-authored-by: Tim Lucas <t@toolmantim.com>

added hover text

6bd2d2e

cla-bot bot added the cla-signed label May 16, 2023

mrnugget requested a review from a team May 17, 2023 08:14

philipp-spiess previously requested changes May 17, 2023

View reviewed changes

deepak2431 added 2 commits May 18, 2023 23:32

updated css, added changelog

9d06103

fixed changelog

343a842

philipp-spiess requested review from philipp-spiess and a team May 19, 2023 09:28

philipp-spiess reviewed May 19, 2023

View reviewed changes

philipp-spiess requested review from philipp-spiess, mrnugget and a team May 23, 2023 15:52

Merge branch 'main' of https://github.com/sourcegraph/sourcegraph int…

b001fe5

…o deepak/cody-intent-highlight

deepak2431 added 2 commits May 30, 2023 22:46

updated css

f8a46aa

Merge branch 'main' of https://github.com/sourcegraph/sourcegraph int…

8b49ea9

…o deepak/cody-intent-highlight

deepak2431 added 2 commits June 2, 2023 10:47

Merge branch 'main' of https://github.com/sourcegraph/sourcegraph int…

c0790d2

…o deepak/cody-intent-highlight

fixed tests

1626266

toolmantim reviewed Jun 4, 2023

View reviewed changes

Merge branch 'main' of https://github.com/sourcegraph/sourcegraph int…

824338f

…o deepak/cody-intent-highlight

updated css, content

89569fa

deepak2431 mentioned this pull request Jul 11, 2023

Cody: Hallucination files detected not correct sourcegraph/cody#88

Closed

toolmantim approved these changes Jun 6, 2023

View reviewed changes

Merge branch 'main' into deepak/cody-intent-highlight

fc01683

toolmantim mentioned this pull request Jun 7, 2023

Cody: Open files in workspace #53069

Merged

deepak2431 added 2 commits June 8, 2023 10:59

resolved conflicts

98c83e5

resolved conflicts

418f7b0

toolmantim reviewed Jun 8, 2023

View reviewed changes

updated css

26d44fe

toolmantim approved these changes Jun 13, 2023

View reviewed changes

toolmantim added 2 commits June 13, 2023 16:07

Merge branch 'main' into deepak/cody-intent-highlight

b06a2f1

Merge branch 'main' into deepak/cody-intent-highlight

4c204b9

toolmantim merged commit 9af30c3 into sourcegraph:main Jun 14, 2023
12 of 13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cody: Add intent hover text #52029

Cody: Add intent hover text #52029

deepak2431 commented May 16, 2023 •

edited

Loading

philipp-spiess left a comment •

edited

Loading

deepak2431 commented May 17, 2023

philipp-spiess commented May 17, 2023

deepak2431 commented May 17, 2023

deepak2431 commented May 18, 2023

philipp-spiess commented May 19, 2023

philipp-spiess May 19, 2023

deepak2431 May 23, 2023

toolmantim commented May 30, 2023 •

edited

Loading

deepak2431 commented May 30, 2023

deepak2431 commented May 30, 2023

deepak2431 commented May 30, 2023 •

edited

Loading

deepak2431 commented Jun 2, 2023

toolmantim left a comment

toolmantim Jun 4, 2023

toolmantim commented Jun 4, 2023

deepak2431 commented Jun 4, 2023

deepak2431 commented Jun 4, 2023

toolmantim commented Jun 4, 2023

deepak2431 commented Jun 4, 2023

toolmantim commented Jun 4, 2023

deepak2431 commented Jun 5, 2023

deepak2431 commented Jun 8, 2023

toolmantim left a comment

toolmantim Jun 8, 2023

deepak2431 Jun 8, 2023

deepak2431 commented Jun 8, 2023

deepak2431 commented Jun 14, 2023

	const title = token.isHallucinated ? 'Hallucination detected: file does not exist' : 'Open file'
	const title = token.isHallucinated ? 'Hallucination detected: file does not exist' : 'No hallucination detected: file exists'

	text-decoration: 1px dashed underline var(--vscode-testing-iconPassed);
	text-decoration: underline

Cody: Add intent hover text #52029

Cody: Add intent hover text #52029

Conversation

deepak2431 commented May 16, 2023 • edited Loading

Test plan

philipp-spiess left a comment • edited Loading

Choose a reason for hiding this comment

deepak2431 commented May 17, 2023

philipp-spiess commented May 17, 2023

deepak2431 commented May 17, 2023

deepak2431 commented May 18, 2023

philipp-spiess commented May 19, 2023

philipp-spiess May 19, 2023

Choose a reason for hiding this comment

deepak2431 May 23, 2023

Choose a reason for hiding this comment

toolmantim commented May 30, 2023 • edited Loading

deepak2431 commented May 30, 2023

deepak2431 commented May 30, 2023

deepak2431 commented May 30, 2023 • edited Loading

deepak2431 commented Jun 2, 2023

toolmantim left a comment

Choose a reason for hiding this comment

toolmantim Jun 4, 2023

Choose a reason for hiding this comment

toolmantim commented Jun 4, 2023

deepak2431 commented Jun 4, 2023

deepak2431 commented Jun 4, 2023

toolmantim commented Jun 4, 2023

deepak2431 commented Jun 4, 2023

toolmantim commented Jun 4, 2023

deepak2431 commented Jun 5, 2023

deepak2431 commented Jun 8, 2023

toolmantim left a comment

Choose a reason for hiding this comment

toolmantim Jun 8, 2023

Choose a reason for hiding this comment

deepak2431 Jun 8, 2023

Choose a reason for hiding this comment

deepak2431 commented Jun 8, 2023

deepak2431 commented Jun 14, 2023

deepak2431 commented May 16, 2023 •

edited

Loading

philipp-spiess left a comment •

edited

Loading

toolmantim commented May 30, 2023 •

edited

Loading

deepak2431 commented May 30, 2023 •

edited

Loading