add prompt snippets support #234220

legomushroom · 2024-11-19T21:34:10Z

For https://github.com/microsoft/vscode-internalbacklog/issues/5220#issuecomment-2486738649

src/vs/base/common/stream.ts

src/vs/workbench/contrib/chat/browser/attachments/implicitContextAttachment.ts

src/vs/workbench/contrib/chat/browser/chatInputPart.ts

src/vs/workbench/contrib/chat/browser/chatVariables.ts

legomushroom · 2024-11-19T21:58:19Z

src/vs/workbench/contrib/chat/browser/contrib/chatDynamicVariables.ts

 	) {
 		super();
 		this._register(widget.inputEditor.onDidChangeModelContent(e => {
 			e.changes.forEach(c => {
 				// Don't mutate entries in _variables, since they will be returned from the getter
-				this._variables = coalesce(this._variables.map(ref => {
+				this._variables = coalesce(this._variables.map((ref) => {
+					if (c.text === `#file:${ref.filenameWithReferences}`) {


That's the only way I made it work and its not ideal, any better ideas? cc @roblourens @joyceerhl

What is this trying to do? Is this triggered when that text is inserted all at once?

When we add the (+N more) label to a filename after nested references are resolved, this is run and as the result the variables are removed. The logic below infers that the variables text have changed and are hence the variables are no longer valid and need to be removed.

Because we update the variable objects when we update the labels, this if check works, - if the variable text is really the same as the variable should have, do nothing.

Can you set a flag during the time that you are making that edit, which says to ignore any onDidChangeModelContent that changes the variable text? Assuming this whole thing is sync.

Otherwise, I guess this is ok, but we could figure out how to make it more obvious what it's doing

legomushroom · 2024-11-19T22:02:17Z

src/vs/workbench/contrib/chat/browser/contrib/chatDynamicVariables.ts

-								endLineNumber: ref.range.endLineNumber,
-								endColumn: ref.range.endColumn + delta
-							}
+						ref.range = {


@roblourens do you recall the main reason for the // Don't mutate entries in _variables.. above? Because the ref is now a class instance with event listeners now, I do need to mutate the variables here 🤔

Because those objects will have been previously returned, and other objects will be holding on to them, and not expecting them to change. Maybe there is somewhere that we diff the previous object with the new one, and mutating the object breaks that. There are other ways we can solve that. But I don't quite understand the problem for you

Maybe there is somewhere that we diff the previous object with the new one

But that would break only if the diff logic relies on the object pointers, doing diffing by-attribute should be unaffected in either case?

There is no really a problem for me, just wanted to make sure that mutating the objects is fine here. We do need to mutate them now because the ref variable is a class instance now rather than a simple object it was before. The class instance can have event listeners registered on it by other code, and it handles nested file references resolution which we don't want to again over and over 🤷

But that would break only if the diff logic relies on the object pointers, doing diffing by-attribute should be unaffected in either case?

The attributes would change, and break the diffing. So we just need to figure out where this matters, and make sure that code can handle a mutable object. I don't know where that is off the top of my head, I can help figure it out later.

src/vs/workbench/contrib/chat/common/chatRequestParser.ts

src/vs/workbench/contrib/chat/common/chatParserTypes.ts

roblourens

Not done reviewing but wanted to drop the first round of comments. Overall, when I'm working on a large feature, I usually try to start pushing small PRs to main as early as possible. That's the best way to avoid merge conflicts and get feedback early. Don't be afraid of pushing an incomplete feature behind a hidden setting, as long as it won't break anything. We do that often on this team. I also would rather avoid PRs that make multiple unrelated changes. It just gets a bit hard to review.

src/vs/base/common/assertDefined.ts

roblourens · 2024-11-20T09:41:22Z

src/vs/base/common/stream.ts

@@ -186,7 +186,7 @@ export interface ITransformer<Original, Transformed> {
 	error?: IErrorTransformer;
 }

-export function newWriteableStream<T>(reducer: IReducer<T>, options?: WriteableStreamOptions): WriteableStream<T> {
+export function newWriteableStream<T>(reducer: IReducer<T> | null, options?: WriteableStreamOptions): WriteableStream<T> {


Optional argument? We generally use undefined over null

Used null on purpose to make it explicit to prevent subtle silent errors. It is too easy to overlook a variable or a function return value being an undefined and pass it over, hence I wanted to make sure the folks that using this to be very specific.

I do appreciate the work that TS has done with void and not treating the foo?: string and foo: undefined | string the same anymore, but there is still the cases when requiring the explicit null saves some hair on your head 🤗 Do you think it would be fine with undefined? You have more context so happy to revert if you think it won't be a pain in the future.

src/vs/workbench/contrib/chat/browser/attachments/implicitContextAttachment.ts

src/vs/workbench/contrib/chat/browser/chatInputPart.ts

src/vs/workbench/contrib/chat/browser/chatVariables.ts

src/vs/workbench/contrib/chat/browser/chatWidget.ts

src/vs/workbench/common/codecs/chatbotPromptCodec/tokens/fileReference.ts

src/vs/workbench/contrib/chat/browser/promptFileReference.ts

src/vs/workbench/contrib/chat/test/common/utils/randomInt.ts

roblourens · 2024-11-20T18:17:55Z

src/vs/workbench/contrib/chat/browser/promptFileReference.ts

+		}
+
+		if (typeof value === 'string') {
+			return value.trim().toLowerCase() === 'true';


Why is this also expecting a string vs boolean?

The returned value is unknown and we provide the JSON view of the config, so users may add the "true"(or even "True") value there and get confused? Or we already parse it to a correct boolean value under the hood?

If you expect the value to be boolean, and they set a string, then I'd say that's wrong and you can ignore it. Or just check whether the value is truthy. It's a temporary unregistered setting, it doesn't really matter

A thruthy value check might be even more confusing as all the [] / {} / random string or non-zero number will be evaluated to true 🤷

src/vs/workbench/contrib/chat/browser/contrib/chatInputEditorContrib.ts

src/vs/workbench/contrib/chat/browser/promptFileReference.ts

roblourens · 2024-11-20T18:34:13Z

I can't get this to work- I set the setting, should I see intellisense for #file? If I manually type #file:./test.js, it doesn't include that file.

Also, it seems like this might break normal implicit context?

…the first unit test

legomushroom · 2024-11-22T01:31:02Z

@roblourens

I can't get this to work- I set the setting, should I see intellisense for #file? If I manually type #file:./test.js, it doesn't include that file.

You mean autocompletion? Yeah it should work just fine, I don't recall it to broken at any point 🤔

Also, it seems like this might break normal implicit context?

It was broken for a bit a while back but should work just fine now, please try again!

src/vs/workbench/contrib/chat/browser/attachments/implicitContextAttachment.ts

src/vs/workbench/contrib/chat/browser/contrib/chatDynamicVariables.ts

src/vs/workbench/contrib/chat/browser/contrib/chatInputEditorContrib.ts

src/vs/workbench/contrib/chat/browser/chatVariables.ts

src/vs/workbench/contrib/chat/common/codecs/chatbotPromptCodec/chatbotPromptDecoder.ts

…s for now

roblourens · 2024-11-23T17:55:03Z

src/vs/workbench/contrib/chat/browser/contrib/chatDynamicVariable.ts

+	);
+
+	if (typeof data === 'string') {
+		return URI.parse(data);


Is this a safe assumption? When is it a stringified URI?

Not really sure, the type seem to be too overreaching and it's not clear why, so my assumption was that a string value here is a path. Can restrict it to URI and Location values only 🤔

Actually not sure what is better in this case - throw immediately if value is a string, or try to parse and throw if it is not an URI. In the latter case we at least trying to do something about it 🤷

The type has been stretched and needs refactoring. But all the instances of this come from our code so you can cover the cases that really exist and ignore the rest or cover them with asserts or whatever you want. But it's confusing to read this because other variables (not "dynamic") have a string value which would be your selected code or something like that. And because I don't know why any variable would have a string URI value, that seems very wrong

I'm confused by this and definitely don't know all the cases when the string value would be a valid URI, but assumed that the parse will throw if it is not. What would you suggest here besides trying to infer if the string is a valid URI manually? Should we reject the string values all together?

src/vs/workbench/contrib/chat/browser/contrib/chatDynamicVariables.ts

roblourens · 2024-11-23T18:00:56Z

src/vs/workbench/contrib/chat/browser/contrib/chatDynamicVariables.ts

+		// nested file references immediatelly and subscribe to updates
+		if (variable.isPromptSnippetFile) {
+			// subscribe to variable changes
+			this._register(variable.onUpdate(() => {


Seems like the wrong lifecycle, these listeners will never be disposed as long as the chat widget exists

Added logic to dispose these variables when they are removed from the _variables list.

We have to add to disposables here anyway. I've pushed the change to remove the closure so that at least the lexical scope is not captured for each of the variables.

We would still have the even listener in the disposables list until chat is disposed, but that seem to be a common issue we have to deal with in this PR 🤔

We have to add to disposables here anyway

Why?

Why?

I've written this and then had the second thought - you are right we don't have to since we now dispose the variables explicitly 🙃

roblourens · 2024-11-23T18:12:53Z

I'm trying to just understand how it's working, I have the setting enabled, but nothing special seems to happen. I'm expecting to see the text saying that there's +1 reference or something like that, I don't get it

…namicVariableModel`

legomushroom · 2024-11-25T17:42:32Z

I'm trying to just understand how it's working, I have the setting enabled, but nothing special seems to happen. I'm expecting to see the text saying that there's +1 reference or something like that, I don't get it

@roblourens only the .prompt.md files work atm, we would ignore the .js references 🤗

roblourens · 2024-11-26T17:49:54Z

only the .prompt.md files work atm, we would ignore the .js references

Do you plan to enable things like that in the future? Seems useful

legomushroom · 2024-11-26T18:26:19Z

@roblourens yeah I believe so but we need to collect more feedback and understand the use cases first. This iteration meant to rely on the separate file extensions, but it would be trivial to remove/adjust this restriction in the future 👍

legomushroom requested review from roblourens and joyceerhl November 19, 2024 21:34

legomushroom self-assigned this Nov 19, 2024

vs-code-engineering bot added this to the November 2024 milestone Nov 19, 2024