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

[prompt instructions]: initial implementation #237604

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

legomushroom
Copy link
Member

For #5292

@legomushroom legomushroom self-assigned this Jan 9, 2025
@vs-code-engineering vs-code-engineering bot added this to the January 2025 milestone Jan 9, 2025
@8153764
Copy link

8153764 commented Jan 9, 2025

For #5292

@8153764
Copy link

8153764 commented Jan 9, 2025

main

@roblourens
Copy link
Member

Seems like it doesn't re-layout the widget after I add prompt instructions
image

@roblourens
Copy link
Member

Maybe the lightbulb icon on the widget should be yellow?

@roblourens
Copy link
Member

When I remove the instructions attachment, I see

ERR Maximum call stack size exceeded: RangeError: Maximum call stack size exceeded
    at dispose (vscode-file://vscode-app/Users/roblou/code/vscode/out/vs/base/common/lifecycle.js:234:23)
    at DisposableStore.clear (vscode-file://vscode-app/Users/roblou/code/vscode/out/vs/base/common/lifecycle.js:325:13)
    at DisposableStore.dispose (vscode-file://vscode-app/Users/roblou/code/vscode/out/vs/base/common/lifecycle.js:309:14)
    at ChatInstructionsAttachmentModel.dispose (vscode-file://vscode-app/Users/roblou/code/vscode/out/vs/base/common/lifecycle.js:398:21)
    at ChatInstructionsAttachmentModel.dispose (vscode-file://vscode-app/Users/roblou/code/vscode/out/vs/workbench/contrib/chat/browser/chatAttachmentModel/chatInstructionsAttachment.js:196:15)
    at DisposableMap.deleteAndDispose (vscode-file://vscode-app/Users/roblou/code/vscode/out/vs/base/common/lifecycle.js:638:31)
    at UniqueContainer.value (vscode-file://vscode-app/Users/roblou/code/vscode/out/vs/workbench/contrib/chat/browser/chatAttachmentModel/chatInstructionAttachmentsModel.js:92:30)
    at Emitter._deliver (vscode-file://vscode-app/Users/roblou/code/vscode/out/vs/base/common/event.js:966:22)
    at Emitter._deliverQueue (vscode-file://vscode-app/Users/roblou/code/vscode/out/vs/base/common/event.js:977:18)
    at Emitter.fire (vscode-file://vscode-app/Users/roblou/code/vscode/out/vs/base/common/event.js:1000:18)

@roblourens
Copy link
Member

I also saw this layout once

image

/**
* Default prompt instructions source folder paths.
*/
const PROMPT_FILES_DEFAULT_LOCATION = ['.copilot/prompts'];
Copy link
Member

Choose a reason for hiding this comment

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

Just wondering about the name, in the UI it says "Instructions", but then the file is a "prompt"

Copy link
Member Author

Choose a reason for hiding this comment

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

It is not a file name but a folder where prompt instruction files (any .md file) are searched for.

We decided to use this path since it is short and more generic hence allows for different prompt file types to be stored here (e.g., instructions, snippets, etc.). This is all transient though, so I bet it will change couple of times in the nearest future 🤷

text-decoration: none;
}
.chat-attached-context .chat-prompt-instructions-attachment .chat-implicit-hint {
opacity: 0.7;
Copy link
Member

Choose a reason for hiding this comment

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

If this is meant to look like the implicit widget, would it make sense to share the styles?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah shared styles is what I had in the beginning but then decided that having separate ones is safer, so CSS changes a bit more isolated between components. Ideally we would have a set of common base styles shared between all similar components, but I didn't dare to do that refactoring in this PR reduce the PR scope 🤗

}
.chat-attached-context .chat-prompt-instructions-attachment.disabled {
border-style: dashed;
opacity: 0.75;
Copy link
Member

@roblourens roblourens Jan 10, 2025

Choose a reason for hiding this comment

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

Why does this get a lower opacity when disabled when the implicit widget doesn't? I might have avoided doing that in the implicit widget for color contrast accessibility reasons.

Copy link
Member Author

@legomushroom legomushroom Jan 10, 2025

Choose a reason for hiding this comment

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

Thought it is a good idea to have a dimmed reference if it is disabled since it is a common UI language users are used to and to reduce visual noise 🤔
The 0.75 value is larger than some of the elements in the "normal" mode (e.g., the .chat-implicit-hint above) so the element remains prominent enough to not cause significant issues 🔍 Will keep it for now but will hold my ear to the ground for more feedback 👂

@roblourens
Copy link
Member

Just a few questions, however none of my comments are blocking. Feel free to check it in! Looks nice

roblourens
roblourens previously approved these changes Jan 10, 2025
@legomushroom
Copy link
Member Author

@roblourens

Seems like it doesn't re-layout the widget after I add prompt instructions

Yeah noticed the issue couple of times, but not sure what's causing this, will keep my eyes peeled 🔍

Maybe the lightbulb icon on the widget should be yellow?

Yes, I great idea! Made it yellow ✅

When I remove the instructions attachment, I see ERR Maximum call stack size exceeded..

Fixed 💃

@legomushroom legomushroom enabled auto-merge (squash) January 10, 2025 05:31
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.

4 participants