Custom chat render for agent skills #286656
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds custom rendering for agent skills in chat by displaying the skill name instead of the file path. The implementation introduces a caching layer for agent skills to enable synchronous lookups by URI.
Key changes:
- Adds a new
getSkillByUrimethod toIPromptsServicefor synchronous skill lookups - Implements a
cachedSkillsByUricache that is populated whenfindAgentSkillsis called - Updates
InlineAnchorWidgetto display skill names instead of file paths when rendering skill references
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/vs/workbench/contrib/chat/common/promptSyntax/service/promptsService.ts | Adds the getSkillByUri interface method with documentation |
| src/vs/workbench/contrib/chat/common/promptSyntax/service/promptsServiceImpl.ts | Implements skill caching with ResourceMap and the getSkillByUri method |
| src/vs/workbench/contrib/chat/browser/widget/chatContentParts/chatInlineAnchorWidget.ts | Injects IPromptsService and uses skill name as label when available |
| // Update the cached skills map for synchronous lookup | ||
| this.cachedSkillsByUri.clear(); | ||
| for (const skill of result) { | ||
| this.cachedSkillsByUri.set(skill.uri, skill); | ||
| } |
There was a problem hiding this comment.
The cache is only populated when findAgentSkills is called, but there's no mechanism to invalidate or refresh the cache when skill files change. If a skill file is added, removed, or modified after findAgentSkills is initially called, getSkillByUri will continue returning stale data. Consider adding file watchers to invalidate the cache when skill files change, or implementing a cache refresh mechanism similar to how cachedCustomAgents.refresh() works for custom agents.
There was a problem hiding this comment.
TODO: I think we should support skills as a promptType and leverage the existing watcher mechanisms there?
| public getSkillByUri(uri: URI): IAgentSkill | undefined { | ||
| return this.cachedSkillsByUri.get(uri); | ||
| } |
There was a problem hiding this comment.
The new getSkillByUri method lacks test coverage. Since there are comprehensive tests for findAgentSkills in promptsService.test.ts, tests should be added to verify that getSkillByUri correctly returns cached skills after findAgentSkills is called, and returns undefined when the cache is empty or the URI is not found.
There was a problem hiding this comment.
See above. We should wait until things are more unified before testing here.
|
Agree we need something like 'getSkillByUri'. |
|
Closed in favour of #286839 |
This affects all file uri renderings for uris that are agent skills. Alternatively, I could pass a flag from the uri itself to signal to render as a skill (and with the skill name), but I think it is cleaner to handle it in the general case here so that we can easily extend to other types in the future.