Use correct file sources instead of always showing 'embeddings' in chat context #2408

DanTup · 2023-12-15T18:24:18Z

Fixes #2244

fyi @kalanchan

Test plan

Select text in an editor and run a command like /explain that will include the context file from the editor
Hover and ensure the tooltip says the file was from the editor, not embeddings
Ask something that will cause the readme to be included automatically
However and ensure the tooltip does not say embeddings

…at context Fixes #2244

dominiccooney

Looks good, but are we missing the 'search' source for symf?

DanTup · 2023-12-22T13:35:40Z

Apparently I didn't submit my reply here.... Should there be a new entry in ContextFileSource?:

cody/lib/shared/src/codebase-context/messages.ts

Lines 6 to 11 in bcd49b3

    
           // tracked for telemetry purposes. Which context source provided this context file. 
        
           // embeddings: context file returned by the embeddings client 
        
           // user: context file provided by the user explicitly via chat input 
        
           // keyword: the context file returned from local keyword search 
        
           // editor: context file retrieved from the current editor 
        
           export type ContextFileSource = 'embeddings' | 'user' | 'keyword' | 'editor' | 'filename' | 'unified'

Or should it use an existing one like keyword?

This test was verifying the user-selected files showed up in the context widget, but this was not intended (see first line of #1858). They were still showing up because the source was not passed through correctly. Now it is, they don't show.

DanTup · 2023-12-29T12:38:44Z

This change causes user-selected files to not show up in the context widget (before/after):

This seemed like a bug, but turns out to be deliberate (see first line of #1858) but wasn't working before because the source wasn't propagated to the chat correctly. Assuming the original reasoning still stands, I've removed the test that was trying to verify these appeared (which I added in another PR recently).

(@toolmantim @abeatrix let me know if I've made the right call here, or if you think we should include them here now)

abeatrix

i think you've made the right call. Left a question regarding ContextFileSource for symf but other than that, looks good to me!

abeatrix · 2023-12-29T14:52:34Z

vscode/src/chat/chat-view/SimpleChatPanelProvider.ts

@@ -1187,6 +1187,8 @@ class ContextProvider implements IContextProvider {
                return {
                    uri: displayUri,
                    range,
+                    // TODO(dantup): Set source (a value from `ContextFileSource`) here.
+                    // source: 'user',


maybe we can create a new ContextFileSource called 'symf' since the results returned from symf is not determined by the user manually? wdyt?

Yeah, that's what the comment above was about - do you know if a) it's safe for me to just add new values here (the type says they're used for analytics - I don't know if they need to match something at the other end), and b) whether any of the existing values are valid for symf or it needs a new one?

Thanks!

@DanTup

a) it's safe for me to just add new values here (the type says they're used for analytics - I don't know if they need to match something at the other end)

That should be fine since the value can be any, i think the key is the important part haha

b) whether any of the existing values are valid for symf or it needs a new one?

I'd say we will need a new one because I don't see it fitting into any of the existing options: 'embeddings' | 'user' | 'keyword' | 'editor' | 'filename' | 'unified' , or do you find one they you think it'd match?

I don't, but I wasn't completely sure about the meaning of keyword and unified. I'll add symf, thanks!

Oh, I just realised that Dominic suggested "search" and this shows up in the tooltip (plus some other things were renamed from "symf" to "search" according to the changelog), so I went with "search".

Oh yeah good points! For reference:

filename is referring to the results from the FilenameContextFetcher

keyword refers to the results from the keywordContextFetcher but i can't find it anymore, so I guess it's been removed and replaced by symf

unified refers to the results from the unifiedContextFetcher that I believe only Cody Web uses. We don't use it in VS Code.

DanTup · 2023-12-29T16:15:04Z

@abeatrix let me know if using "search" (as noted above) seems reasonable, and if so (and the bots are green) I'll merge this. Thanks!

abeatrix · 2023-12-29T16:19:59Z

@abeatrix let me know if using "search" (as noted above) seems reasonable, and if so (and the bots are green) I'll merge this. Thanks!

Yea I think that makes sense to me 👍

DanTup · 2023-12-29T18:03:34Z

@dominiccooney merging is blocked on your review. I believe my recent change resolved your issue, but let me know if anything else needs changing. Thanks!

DanTup · 2023-12-29T19:29:22Z

@abeatrix actually, now I'm a little confused.

In the top line at #1858 it says:

the context display widget should also include local context except the ones added by the user manually via @

And with my fix in this PR, the user-chosen files indeed don't show up. However, in the screenshots (eg. #1858 (comment)) it does still show the mentioned file - so I'm not certain what the intended functionality is now.

If the user types something like "Explain foo.dart", should the context widget show up with "foo.dart" listed in it?

abeatrix · 2023-12-29T19:38:09Z

in the screenshots (eg. #1858 (comment)

@DanTup Oh that's because the screenshot you linked is context added for the explain command, which was not added by the user manually via @ . In those cases, I believe Tim wanted to include those under context last time we chatted on slack here: https://sourcegraph.slack.com/archives/C05AGQYD528/p1700638221936799?thread_ts=1700622656.385639&cid=C05AGQYD528

abeatrix · 2023-12-29T19:40:26Z

But if the context was added via manual @ input, then i believe we do not want to include those in the context list if i'm not misunderstanding tim's message here: https://sourcegraph.slack.com/archives/C05AGQYD528/p1700631673836569?thread_ts=1700622656.385639&cid=C05AGQYD528

DanTup · 2024-01-05T14:41:35Z

Great, thanks for confirming. I've re-merged main so it's all up-to-date. Needs @dominiccooney's stamp before it'll let me merge (I presume he's out this week).

staled

abeatrix · 2024-01-08T20:02:21Z

Dismissed staled review request to unblock.

DanTup · 2024-01-09T10:45:43Z

Thanks! :)

DanTup added 3 commits December 15, 2023 18:22

Use correct file sources instead of always showing 'embeddings' in ch…

7cb5362

…at context Fixes #2244

Update CHANGELOG.md

30a21b7

Fix test

10921a5

dominiccooney previously requested changes Dec 21, 2023

View reviewed changes

DanTup added 2 commits December 22, 2023 12:25

Merge branch 'main' into dantup/fix-missing-context-sources

32c8af2

Add a TODO

85f73b3

DanTup added 2 commits December 29, 2023 11:50

Merge branch 'main' into dantup/fix-missing-context-sources

c6a95e7

Remove redundant test

c1fb8cf

This test was verifying the user-selected files showed up in the context widget, but this was not intended (see first line of #1858). They were still showing up because the source was not passed through correctly. Now it is, they don't show.

DanTup mentioned this pull request Dec 29, 2023

Fix clicking on context files with absolute paths on Windows #2400

Closed

abeatrix approved these changes Dec 29, 2023

View reviewed changes

Set context source for symf results

edba9a3

DanTup enabled auto-merge (squash) December 29, 2023 18:03

DanTup mentioned this pull request Dec 29, 2023

bug: Chat context files sometimes doesn't include explicitly mentioned files #2417

Closed

DanTup disabled auto-merge December 29, 2023 19:27

DanTup added 2 commits January 5, 2024 14:10

Merge branch 'main' into dantup/fix-missing-context-sources

4e17bb2

Fix formatting

5ece6e6

Merge branch 'main' into dantup/fix-missing-context-sources

24c1dbf

fix merge issues

9edd349

abeatrix merged commit b552a9a into main Jan 8, 2024
16 checks passed

abeatrix deleted the dantup/fix-missing-context-sources branch January 8, 2024 20:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use correct file sources instead of always showing 'embeddings' in chat context #2408

Use correct file sources instead of always showing 'embeddings' in chat context #2408

DanTup commented Dec 15, 2023 •

edited

dominiccooney left a comment •

edited

DanTup commented Dec 22, 2023

DanTup commented Dec 29, 2023

abeatrix left a comment

abeatrix Dec 29, 2023

DanTup Dec 29, 2023

abeatrix Dec 29, 2023

DanTup Dec 29, 2023

DanTup Dec 29, 2023

abeatrix Dec 29, 2023

DanTup commented Dec 29, 2023

abeatrix commented Dec 29, 2023

DanTup commented Dec 29, 2023

DanTup commented Dec 29, 2023

abeatrix commented Dec 29, 2023

abeatrix commented Dec 29, 2023

DanTup commented Jan 5, 2024

abeatrix commented Jan 8, 2024

DanTup commented Jan 9, 2024

Use correct file sources instead of always showing 'embeddings' in chat context #2408

Use correct file sources instead of always showing 'embeddings' in chat context #2408

Conversation

DanTup commented Dec 15, 2023 • edited

Test plan

dominiccooney left a comment • edited

Choose a reason for hiding this comment

DanTup commented Dec 22, 2023

DanTup commented Dec 29, 2023

abeatrix left a comment

Choose a reason for hiding this comment

abeatrix Dec 29, 2023

Choose a reason for hiding this comment

DanTup Dec 29, 2023

Choose a reason for hiding this comment

abeatrix Dec 29, 2023

Choose a reason for hiding this comment

DanTup Dec 29, 2023

Choose a reason for hiding this comment

DanTup Dec 29, 2023

Choose a reason for hiding this comment

abeatrix Dec 29, 2023

Choose a reason for hiding this comment

DanTup commented Dec 29, 2023

abeatrix commented Dec 29, 2023

DanTup commented Dec 29, 2023

DanTup commented Dec 29, 2023

abeatrix commented Dec 29, 2023

abeatrix commented Dec 29, 2023

DanTup commented Jan 5, 2024

abeatrix commented Jan 8, 2024

DanTup commented Jan 9, 2024

DanTup commented Dec 15, 2023 •

edited

dominiccooney left a comment •

edited